Uh oh!
There was an error while loading. Please reload this page.
feat(nexus): add async, cancellation, failure and parallel features - #871
feat(nexus): add async, cancellation, failure and parallel features#871xepozz wants to merge 2 commits into
Conversation
Four Nexus scenarios implemented in Go and Java: an async workflow-run operation, cancelling a running async operation, a sync operation that fails with an application error, and three parallel sync operations.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:d5d438ca6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| return runner.Client.ExecuteWorkflow(ctx, opts, Workflow, runner.NexusEndpoint) | ||
| }, | ||
| CheckHistory: func(ctx context.Context, runner *harness.Runner, run client.WorkflowRun) error { |
There was a problem hiding this comment.
When this feature runs, assigning CheckHistory replaces the harness path that calls Runner.CheckHistoryDefault (harness/go/harness/runner.go lines 151-157), so neither the current executions—including the backing workflow—nor stored histories are replayed. Run the default checker after the event assertion; the same omission occurs in the other three newly added Go Nexus features.
Useful? React with 👍 / 👎.
| } | ||
| @Override | ||
| public void checkHistory(Runner runner, Run run) throws Exception { |
There was a problem hiding this comment.
Overriding checkHistory here bypasses the default implementation in Feature.java, which calls runner.checkCurrentAndPastHistories(run). Consequently this feature never replay-checks its caller or backing workflow and cannot detect compatibility regressions against saved histories; invoke the default replay helper after the custom assertion. The other three new Java Nexus features have the same omission.
Useful? React with 👍 / 👎.
| .getResult() | ||
| .handle( | ||
| (value, failure) -> { | ||
| resultHolder[0] = failure == null ? "completed" : "cancelled"; |
There was a problem hiding this comment.
When the operation result fails for any reason after scope.cancel()—for example a timeout or backing-workflow application failure—this callback labels it "cancelled", and the workflow plus history assertion still pass. Since the feature specifically promises to observe a cancellation error, inspect the failure chain for CanceledFailure rather than treating every non-null failure as cancellation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a good flag, in the Go test we do check for this already
The java handler threw a bare application failure, which the SDK reports as a handler error, so the two languages were asserting different contracts.
| .build(); | ||
| TestService stub = Workflow.newNexusServiceStub(TestService.class, serviceOptions); | ||
| var resultHolder = new String[1]; |
There was a problem hiding this comment.
| varresultHolder = newString[1]; | |
| varresultHolder = Workflow.newPromise(); |
| return OperationHandler.sync( | ||
| (context, details, name) -> { | ||
| throw OperationException.failure( | ||
| ApplicationFailure.newNonRetryableFailure("deliberate failure", "TestError")); |
There was a problem hiding this comment.
In the Go test temporal.NewApplicationError(ErrorMessage, ErrorType) this will return a retry able application error
There was a problem hiding this comment.
The two tests should be consistent
| @@ -0,0 +1,13 @@ | |||
| # Nexus sync operation fails | |||
| A workflow invokes a synchronous Nexus operation that raises an application failure and | |||
There was a problem hiding this comment.
The Nexus operations you wrote do not "raises an application failure", they raise a OperationError. The OperationError happens to contain a application failure as a cause but from the SDKs perspective that is not relevant. What you have tested is fine, but I would correct the spec.
I'll note having a test where the Nexus operation DOES raise a application failure would also be a good test, but it doesn't have to be in this PR
| @@ -0,0 +1,12 @@ | |||
| # Nexus operations run in parallel | |||
| A workflow starts three synchronous Nexus operations in a single workflow task and awaits all | |||
There was a problem hiding this comment.
Can you please change the folder name to clarify this is parallel sync operations.
Four Nexus scenarios implemented in Go and Java: an async workflow-run operation, cancelling a running async operation, a sync operation that fails with an application error, and three parallel sync operations.
What was changed
Why?
Checklist
Closes
How was this tested: