Nexus Standalone: wire up#9837
Conversation
2253f0c to
b503714
Compare
10d3595 to
83bfb53
Compare
b503714 to
56c2d98
Compare
83bfb53 to
6a0bd6e
Compare
56c2d98 to
517d0bc
Compare
6a0bd6e to
88281af
Compare
f69fecd to
7094ffa
Compare
| }) | ||
|
|
||
| t.Run("ReturnsLastAttemptFailure", func(t *testing.T) { | ||
| t.Skip("Enable once standalone Nexus task and cancellation executors are wired through public Nexus task APIs") |
There was a problem hiding this comment.
Removed this (was slightly mislabelled)
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Skip("Enable once standalone Nexus task and cancellation executors are wired through public Nexus task APIs") |
There was a problem hiding this comment.
Removed this (was slightly mislabelled)
470834e to
643f340
Compare
22ed5f4 to
8b51e02
Compare
b936601 to
103b0e1
Compare
8b51e02 to
d13cafb
Compare
| }, | ||
| expectedStatus: enumspb.NEXUS_OPERATION_EXECUTION_STATUS_CANCELED, | ||
| expectedFailureMessage: "cancel failure", | ||
| }, |
There was a problem hiding this comment.
New test since cancellation is in now
| if o.Status == nexusoperationpb.OPERATION_STATUS_TIMED_OUT && o.LastAttemptFailure != nil { | ||
| return nil, o.LastAttemptFailure | ||
| } |
There was a problem hiding this comment.
This is the special case added here
There was a problem hiding this comment.
If the operation fails with a timeout error, we still have the last attempt failure in the describe response, but that's different from the cause of the operation failure.
There was a problem hiding this comment.
I must have misunderstood our standup conversation about this from a while ago; I'll remove it.
| return nil, err | ||
| } | ||
| return commonnexus.NexusFailureToTemporalFailure(nexusFailure) | ||
| } |
There was a problem hiding this comment.
NexusTestEnv has a helper for this but the test hasn't been converted to use it yet; follow up ...
|
|
||
| message StartNexusOperationRequest { | ||
| string namespace_id = 1; | ||
| string endpoint_id = 2; |
There was a problem hiding this comment.
"breaking change"
Add API boilerplate for standalone Nexus Operations. - [x] built - [ ] run locally and tested manually - [x] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s)
Add Nexus Standalone feature flag. Tests will be added to respective API impl.
Add Nexus Standalone Describe and Start handlers. - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [x] added new unit test(s) - [x] added new functional test(s) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add Nexus Standalone List and Count handlers. - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [x] added new functional test(s)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
||
| switch { | ||
| case !hasOutcome: | ||
| return nil, o.LastAttemptFailure |
There was a problem hiding this comment.
My only concern with how the code is structured is that it assumes that the caller of this function checked that the operation is closed already. Otherwise the last attempt as outcome would be incorrect.
There was a problem hiding this comment.
The method had that issue before this refactor, right (it fell back to o.LastAttemptFailure)?
But I agree, that seems wrong - I'll make it return nil instead.
There was a problem hiding this comment.
This originated in SAA; I see the outcome method there falls back to LastAttempt if there is no success/failure - but only if it's closed. I wonder what the use case is.
Do you think it's safe to assume that a closed operation will always have success/failure outcome set? Or do we need the fallback to LastAttemptFailure like SAA has?
There was a problem hiding this comment.
We need to fallback to LastAttemptFailure. My point was about the function should be self-contained and not make an assumption on how it is called.
| if o.Status == nexusoperationpb.OPERATION_STATUS_TIMED_OUT && o.LastAttemptFailure != nil { | ||
| return nil, o.LastAttemptFailure | ||
| } |
There was a problem hiding this comment.
If the operation fails with a timeout error, we still have the last attempt failure in the describe response, but that's different from the cause of the operation failure.
|
|
||
| // Result is set by standalone operations. Workflow-attached operations store | ||
| // the result in the history event and remove the operation after this transition. | ||
| if event.Result != nil { |
There was a problem hiding this comment.
We can just always pass in the result IMHO. It doesn't matter for workflow operations since the operation would be immediately deleted from the tree and not take up any additional storage.
There was a problem hiding this comment.
(which would make this if redundant)
| // Result is set by standalone operations. Workflow-attached operations store | ||
| // the result in the history event and remove the operation after this transition. | ||
| if event.Result != nil { | ||
| o.outcome(ctx).Variant = &nexusoperationpb.OperationOutcome_Successful_{ |
There was a problem hiding this comment.
Maybe call outcome() getOrCreateOutcome() to clarify that this is what happens under the hood?
| } | ||
|
|
||
| var links []nexus.Link | ||
| if args.nexusLink != (nexus.Link{}) { |
There was a problem hiding this comment.
I would just make args.nexusLinks plural and not deal with the empty state. Also there should always be a link, so not sure why this line is needed.
|
|
||
| message StartNexusOperationRequest { | ||
| string namespace_id = 1; | ||
| string endpoint_id = 2; |
Co-authored-by: Roey Berman <roey@temporal.io>
bergundy
left a comment
There was a problem hiding this comment.
Very close now. The comments are blocking but I trust you to merge only after addressing.
There was a problem hiding this comment.
You are missing populating a link for the standalone nexus operation. You need to generate a link in this format in loadStartArgs:
// A link to a standalone Nexus operation.
message NexusOperation {
string namespace = 1;
string operation_id = 2;
string run_id = 3;
}|
|
||
| // Result is set by standalone operations. Workflow-attached operations store | ||
| // the result in the history event and remove the operation after this transition. | ||
| if event.Result != nil { |
There was a problem hiding this comment.
(which would make this if redundant)
| }) | ||
| } | ||
|
|
||
| func TestDescribeOutcome(t *testing.T) { |
There was a problem hiding this comment.
You need a test for when we use the last attempt failure instead of the outcome field.
There was a problem hiding this comment.
@bergundy confused about this one; I had removed (dccc430) the "return last attempt failure as outcome" behavior entirely based on the first review comment.
I'll merge for now to unblock us but happy to address this as a follow-up.
There was a problem hiding this comment.
I clarified the comment above.
…tephanos/ns-standalone-wire-up
What changed?
Wire up Nexus Standalone with CHASM.
How did you test it?