Nexus Standalone: Poll#9780
Conversation
2f7e5d1 to
32a04db
Compare
| enums[i] = T(values.Get(i).Number()) | ||
| } | ||
| return enums | ||
| } |
There was a problem hiding this comment.
seems generally useful for exhaustiveness checks in tests
f3da566 to
46661ef
Compare
c6a4fe7 to
172314b
Compare
40e5047 to
aacc175
Compare
| func TestDescribeStandaloneNexusOperation(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| t.Run("EmptyRunID", func(t *testing.T) { |
There was a problem hiding this comment.
merged this one into StartAndDescribe above
aacc175 to
31b0dce
Compare
191dbbc to
6c867b4
Compare
6c867b4 to
d027a3e
Compare
bergundy
left a comment
There was a problem hiding this comment.
My comments are very minimal. I'm approving but please address before merging.
|
|
||
| var LongPollTimeout = dynamicconfig.NewNamespaceDurationSetting( | ||
| "nexusoperation.longPollTimeout", | ||
| 20*time.Second, |
There was a problem hiding this comment.
We were having discussions on slack for making this default to one minute. Please take note and adjust as we settle the discussion. Not blocking the PR.
chasm/lib/nexusoperation/config.go
Outdated
| var LongPollTimeout = dynamicconfig.NewNamespaceDurationSetting( | ||
| "nexusoperation.longPollTimeout", | ||
| 20*time.Second, | ||
| `Timeout for nexus operation long-poll requests.`, |
There was a problem hiding this comment.
I think this is the maximum timeout if the context deadline is longer, no?
| if errors.Is(err, chasm.ErrMalformedComponentRef) { | ||
| return nil, false, serviceerror.NewInvalidArgument("invalid long poll token") | ||
| } | ||
| if errors.Is(err, chasm.ErrInvalidComponentRef) { | ||
| return nil, false, serviceerror.NewInvalidArgument("long poll token does not match execution") | ||
| } |
There was a problem hiding this comment.
I saw this copied from the SAA implementation and wondering if we need this translation just for nicer errors strings or if there's a way to use the chasm predefined errors directly, e.g., by having more informative error messages on them.
There was a problem hiding this comment.
I'm filing a ticket for this to unblock the merge; giving me more time to hash out a better solution with SAA crew.
| case enumspb.NEXUS_OPERATION_WAIT_STAGE_CLOSED: | ||
| return o.isClosed() | ||
| default: | ||
| return false |
There was a problem hiding this comment.
You should validate these wait stages in the frontend and put a comment here that these are the only ones supported.
The frontend should return an invalid argument error instead of false so if a new client with a new API definition that has another variant here can know that they are running against a server that is too old.
| resp.Outcome = &workflowservice.PollNexusOperationExecutionResponse_Result{ | ||
| Result: successful.GetResult(), | ||
| } | ||
| } else if failure := outcome.GetFailed().GetFailure(); failure != nil { |
There was a problem hiding this comment.
We won't always have the response in the outcome field, sometimes we need to get it from the last attempt failure.
This is relevant for describe too. You can see the SAA code for reference.
| return serviceerror.NewInvalidArgumentf("operation_id exceeds length limit. Length=%d Limit=%d", | ||
| len(req.GetOperationId()), config.MaxIDLengthLimit()) | ||
| } | ||
| if len(req.GetLongPollToken()) > 0 && req.GetRunId() == "" { |
There was a problem hiding this comment.
We should probably validate the long poll token just in case. That's missing in the SAA validator too.
| OperationId: "does-not-exist", | ||
| }) | ||
| var notFound *serviceerror.NotFound | ||
| s.ErrorAs(err, ¬Found) |
There was a problem hiding this comment.
Can you also assert that the error message is user friendly? For example does it say nexus operation not found?
tests/nexus_standalone_test.go
Outdated
| s.ErrorAs(err, ¬Found) | ||
| }) | ||
|
|
||
| t.Run("WrongRunID", func(t *testing.T) { |
There was a problem hiding this comment.
Redundant IMHO, you're testing CHASM at this point.
tests/nexus_standalone_test.go
Outdated
| name: "NonExistentOperationID", | ||
| request: &workflowservice.PollNexusOperationExecutionRequest{ | ||
| Namespace: s.Namespace().String(), | ||
| OperationId: "non-existent-op", | ||
| RunId: startResp.RunId, | ||
| WaitStage: enumspb.NEXUS_OPERATION_WAIT_STAGE_CLOSED, | ||
| }, | ||
| expectedErr: notFoundErr, | ||
| }, | ||
| { | ||
| name: "NonExistentRunID", | ||
| request: &workflowservice.PollNexusOperationExecutionRequest{ | ||
| Namespace: s.Namespace().String(), | ||
| OperationId: "test-op", | ||
| RunId: "11111111-2222-3333-4444-555555555555", | ||
| WaitStage: enumspb.NEXUS_OPERATION_WAIT_STAGE_CLOSED, | ||
| }, | ||
| expectedErr: notFoundErr, |
There was a problem hiding this comment.
These two cases are redundant, the library code doesn't care why an execution is not found, that's the framework's job.
Add polling support via PollNexusOperationExecution and DescribeNexusOperationExecution. - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [x] added new unit test(s) - [x] added new functional test(s)
Add polling support via PollNexusOperationExecution and DescribeNexusOperationExecution. - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [x] added new unit test(s) - [x] added new functional test(s)
Add polling support via PollNexusOperationExecution and DescribeNexusOperationExecution. - [ ] 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>
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>
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>
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>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
What changed?
Add polling support via PollNexusOperationExecution and DescribeNexusOperationExecution.
How did you test it?