NEXUS-485: Support Workflow Update as a Nexus Operation - #2945
Conversation
7852a29 to
93ee1a9
Compare
| .setWorkflowId(workflowID) | ||
| .setRunId(runID)); | ||
| } catch (Exception e) { | ||
| log.error("Failed to parse Nexus link URL", e); |
There was a problem hiding this comment.
Might still want to log the bad URL, like you did above
There was a problem hiding this comment.
Same as before, might have gotten unfixed!
798079e to
d1cd99c
Compare
| // only run this test if the server supports update completion callbacks | ||
| // currently only for local server manual checks until the in-memory test | ||
| // server can support update completion callbacks | ||
| assumeTrue( |
There was a problem hiding this comment.
We should just skip the Update tests for the in-memory test server we should always be running these tests when using a real server for test. You can look at how we tested other things like Stand Alone Activities
There was a problem hiding this comment.
So we won't need UPDATE_CALLBACKS_SUPPORTED
There was a problem hiding this comment.
SDKTestWorkflowRule.useExternalService this wasnt enough as the test also needs update callbacks to be enabled via dynamic config
Didnt want to rely on just local server flag because someone could run server without those flags - they arent required anyway
Added this way to ensure that if someone runs this against their own server- for testing other SDK changes etc- itll only get tested iff the server supports it
Or can add a note if someone runs into this that theyd need to enable flags on their real server
There was a problem hiding this comment.
Yes you should assume the users server has this DC set or fail the test
There was a problem hiding this comment.
To run SDK test against a local server you have to set the require DC, that is true is all SDKs CI
There was a problem hiding this comment.
Got it - will change and just add a comment
| requestLinks)) | ||
| .addAllLinks(requestLinks); | ||
| } else { | ||
| log.error("unexpected error fetching nexusOperationMetadata"); |
There was a problem hiding this comment.
This is an expected case if the user makes a regular update call from the regular Temporal client inside a Nexus handler, we need to make sure that still works like before and we didn't break anything there
d1cd99c to
adda65b
Compare
| @Override | ||
| public <T, A1, A2, A3, A4> TemporalOperationResult<Void> startWorkflowUpdate( | ||
| Class<T> workflowClass, | ||
| String workflowId, |
There was a problem hiding this comment.
I think we need overloads that take the workflow execution as well to target the specific run ID , @maciejdudko thoughts?
There was a problem hiding this comment.
added one just to align on the shape here https://github.com/temporalio/sdk-java/pull/2945/changes#diff-5f53049561675309c6d8a1a6b09638b2e6e9096a5b614047e2d96d4491ed5a73R776
Just so we're on the same page, if we want to do this, there will be 15 more(additional, not replacement) that will have workflow execution instead of the workflowID right?
There was a problem hiding this comment.
Yeah it is a lot of overloads , but we need to be able to target by workflow ID or (Workflow ID, Run ID) pair
| private <R> void checkNexusUpdateOptionsValid(UpdateOptions<R> options) | ||
| throws OperationException { | ||
| if (options.getWaitForStage() != WorkflowUpdateStage.ACCEPTED) { | ||
| throw OperationException.failed( |
There was a problem hiding this comment.
I don't think this should fail the Nexus operation, I thought we already decided that in the Go SDK PR
There was a problem hiding this comment.
updated to handler error to retry
|
|
||
| public CancelUpdateWorkflowInput(String workflowId, String runId, String updateId) { | ||
| this.workflowId = Objects.requireNonNull(workflowId); | ||
| this.runId = Strings.nullToEmpty(runId); |
There was a problem hiding this comment.
Note still need to update this to require
626bcd9 to
1d3395d
Compare
| .setWorkflowId(workflowID) | ||
| .setRunId(runID)); | ||
| } catch (Exception e) { | ||
| log.error("Failed to parse Nexus link URL", e); |
There was a problem hiding this comment.
Same as before, might have gotten unfixed!
This was an intentional change - the idea was that we keep retrying and the user will deploy a new version of the handler that will fix it. Still up for discussion before it goes GA but for the preview, this is consistent across go/python/java
This was more just to be in-line with the existing convention - dont fail if unparsable Should we fix for all? Can you also please comment on this thread #2945 (comment) - unsure about adding 14/15 more overloads but there might be a valid need for it - targeting by runID |
1d3395d to
cf1ba0b
Compare
|
|
Yes, but was also setting the retry explicitly for that reason- iiuc, this will ensure it will retry This case is bit odd because it is a bad request- but is retriable so this seemed like the right way to do it |
|
I'd probably capture the validation failure as |
maciejdudko
left a comment
There was a problem hiding this comment.
Just a few small things, then it should be ready to merge.
| } catch (JsonProcessingException e) { | ||
| throw new IllegalStateException("failed to generate update operation token", e); | ||
| } |
There was a problem hiding this comment.
-
Under what circumstances can
JsonProcessingExceptionhappen? Is it user error or server/implementation error? If it's user error then we ought to have better message. -
This doesn't catch unchecked exceptions.
catch (Exception e)would be a better choice.
There was a problem hiding this comment.
It should only be a server error unless the user is trying to manually craft a new token- but that is not the normal case
| public enum OperationTokenType { | ||
| UNKNOWN(0), | ||
| WORKFLOW_RUN(1); | ||
| WORKFLOW_RUN(1), | ||
| WORKFLOW_UPDATE(3); |
There was a problem hiding this comment.
Value 2 is missing. If it's intentional, a comment would be helpful.
| throws OperationException; | ||
|
|
||
| /** | ||
| * TBD: determine if all the other 14 func+proc+void are required and add if needed Starts a |
There was a problem hiding this comment.
- Change TBD to TODO.
- TBD/TODO should not be part of the docstring. Make it a regular comment before docstring.
| * #startWorkflowUpdate(Class, String, Functions.Func1, UpdateOptions)} for the full behavior | ||
| * contract. | ||
| * | ||
| * @param <T> |
There was a problem hiding this comment.
Doc tag contents are missing for this overload.
cf1ba0b to
2cec87b
Compare
| "invalid update request", | ||
| new IllegalArgumentException( | ||
| "nexus op workflow updates only support WorkflowUpdateStageAccepted for async updates"), | ||
| RetryBehavior.RETRYABLE); |
There was a problem hiding this comment.
If this fails, it will always fail, won't it? So the error shouldn't be RETRYABLE
There was a problem hiding this comment.
This was the one discussed here - #2945 (comment)
The idea was that a new version of the handler could be deployed which fixes the wid and it would then start passing
This is consistent with the go/python SDKs temporalio/sdk-go#2417 (comment)
b1f4a36 to
bb1d3de
Compare
What was changed
Adds support for Workflow Update as a Nexus operation
Why?
Part of effort to expose all Temporal primitives via Nexus operations
Checklist
Closes NEXUS-485
How was this tested: