-
Notifications
You must be signed in to change notification settings - Fork 169
Sdk 4131 #2625
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Sdk 4131 #2625
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements comprehensive Nexus operation support for Temporal's SDK and test server, adding client-side operation management capabilities and external caller task handling.
- Adds
fetchOperationResult
andfetchOperationInfo
methods to Nexus operation handlers - Implements external caller task handling in the test server for non-workflow Nexus operations
- Introduces Nexus service client support with interceptors and OpenTracing integration
Reviewed Changes
Copilot reviewed 48 out of 49 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
TracingWorkerInterceptor.java | Adds tracing support for new fetchOperationResult and fetchOperationInfo methods |
SDKTestWorkflowRule.java | Adds convenience method to create Nexus service clients for testing |
NexusWorkflowTest.java | Updates test to use new NexusWorkflowTaskToken class |
messages.proto | Defines new protobuf message structure for Nexus task tokens |
TestWorkflowStoreImpl.java | Implements external caller task handling and async operation management |
WorkflowRunOperationImpl.java | Implements fetchResult and fetchInfo methods for workflow operations |
NexusServiceClientCallsInterceptor.java | Defines interceptor interface for Nexus service client calls |
WorkflowClient.java | Adds methods to create Nexus service clients and completion clients |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
@@ -509,7 +510,7 @@ public void getWorkflowExecutionHistory( | |||
// context deadline and throw DEADLINE_EXCEEDED if the deadline is less | |||
// than 20s. | |||
// If it's longer than 20 seconds - we return an empty result. | |||
Deadline.after(20, TimeUnit.SECONDS))); | |||
Deadline.after(100, TimeUnit.MILLISECONDS))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deadline has been reduced from 20 seconds to 100 milliseconds. This significant change could cause timeouts in slower test environments and should be documented or reverted to avoid breaking existing tests.
Deadline.after(100, TimeUnit.MILLISECONDS))); | |
Deadline.after(20, TimeUnit.SECONDS))); |
Copilot uses AI. Check for mistakes.
taskQueueId, | ||
pollNexusTaskQueueResponse, | ||
// TODO: Derive the deadline from the context | ||
Timestamps.fromMillis(System.currentTimeMillis() + 1000 * 10) // 10s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are multiple TODO comments for deriving deadlines from context. These should be addressed as hardcoded 10-second timeouts may not be appropriate for all use cases.
Timestamps.fromMillis(System.currentTimeMillis() + 1000 * 10) // 10s | |
long deadlineMillis = System.currentTimeMillis() + deadline.toMillis(); | |
taskQueue.add( | |
new NexusTask( | |
taskQueueId, | |
pollNexusTaskQueueResponse, | |
Timestamps.fromMillis(deadlineMillis) |
Copilot uses AI. Check for mistakes.
bugbot run |
@@ -509,7 +510,7 @@ public void getWorkflowExecutionHistory( | |||
// context deadline and throw DEADLINE_EXCEEDED if the deadline is less | |||
// than 20s. | |||
// If it's longer than 20 seconds - we return an empty result. | |||
Deadline.after(20, TimeUnit.SECONDS))); | |||
Deadline.after(100, TimeUnit.MILLISECONDS))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was changed
Why?
Checklist
Closes
How was this tested: