Add support for attaching to a running workflow #2424
Conversation
|
Marking as draft while I add tests, will need #2415 merged and probably a server release to merge |
| } | ||
|
|
||
| public OnConflictOptions build() { | ||
| return new OnConflictOptions(attachRequestId, attachCompletionCallbacks, attachLinks); |
There was a problem hiding this comment.
The server doesn't allow attaching callbacks without also attaching a request ID (as of yesterday). I'm wondering if we should verify that SDK side and document it on the builder setters. We definitely should in the api (@rodrigozhou).
| * already running, the options specifies the actions to be taken on the running workflow. If | ||
| * not set or use together with any other WorkflowIDConflictPolicy, this parameter is ignored. | ||
| * | ||
| * <p>WARNING: Not intended for User Code. |
There was a problem hiding this comment.
I wish there was a way to hide this. Maybe with a private subclass and cast?
There was a problem hiding this comment.
Can't be private because it needs to be accessible by the Internal package. Passing them through a thread local is an option, but puts a requirement on the calling thread. I think we should revisit before we GA
temporal-sdk/src/main/java/io/temporal/internal/statemachines/NexusOperationStateMachine.java
Show resolved
Hide resolved
| if (!async) { | ||
| startedCallback.apply(Optional.empty(), null); | ||
| } |
There was a problem hiding this comment.
I would just keep all of this logic in notifyStarted() but this is fine.
| + '}'; | ||
| } | ||
|
|
||
| private OnConflictOptions( |
There was a problem hiding this comment.
Pedantic, but a bit strange to me to see a constructor below the instance methods
| * io.temporal.api.enums.v1.WorkflowIdConflictPolicy#WORKFLOW_ID_CONFLICT_POLICY_USE_EXISTING} | ||
| */ | ||
| @Experimental | ||
| public class OnConflictOptions { |
There was a problem hiding this comment.
Hrmm, it was my understanding that we were going to hive this from users, though I know that is difficult in Java. If instead we're ok with showing to users but marking experimental, I think we should do the same in Go.
There was a problem hiding this comment.
We can't avoid exposing it given the structure of the Java SDK, just because the Java SDK has this unfortunate limitation doesn't mean other SDKs should copy it
There was a problem hiding this comment.
So the question becomes should other SDKs follow Go's lead or Java's lead here. Sounds like we are saying Java will be the outlier instead of updating Go to expose as well?
There was a problem hiding this comment.
Yes other SDKs should not copy it
Quinn-With-Two-Ns
force-pushed
the
SDK-2931
branch
from
515b050 to
dc2f6e9
Compare
Add support for attaching to a running workflow.
See similar Go PR: temporalio/sdk-go#1797