-
Notifications
You must be signed in to change notification settings - Fork 157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Worker Versioning Annotations & Options #2463
Worker Versioning Annotations & Options #2463
Conversation
a48d6b9
to
ef1b574
Compare
temporal-sdk/src/main/java/io/temporal/internal/worker/WorkerVersioningOptions.java
Outdated
Show resolved
Hide resolved
temporal-sdk/src/main/java/io/temporal/worker/WorkerOptions.java
Outdated
Show resolved
Hide resolved
ef1b574
to
0e6eefd
Compare
Please make sure you also test Spring boot |
temporal-sdk/src/main/java/io/temporal/internal/worker/WorkerVersioningOptions.java
Outdated
Show resolved
Hide resolved
VersioningBehavior versioningBehavior; | ||
try { | ||
implMethod = | ||
workflowImplementationClass.getMethod(method.getName(), method.getParameterTypes()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic can be move to the metadata
package for implementations
temporal-testing/src/main/java/io/temporal/testing/TestWorkflowRule.java
Outdated
Show resolved
Hide resolved
temporal-sdk/src/test/java/io/temporal/worker/WorkerVersioningTest.java
Outdated
Show resolved
Hide resolved
temporal-sdk/src/main/java/io/temporal/worker/WorkerDeploymentOptions.java
Show resolved
Hide resolved
temporal-sdk/src/main/java/io/temporal/worker/WorkerDeploymentOptions.java
Show resolved
Hide resolved
Do you mean configuring the worker options via config files? |
Yes and that our auto discovery picks up the annotation. Basically that is works E2E. |
return Eventually.assertEventually( | ||
Duration.ofSeconds(15), | ||
() -> { | ||
DescribeWorkerDeploymentResponse resp = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need a high level deployment client? Go has that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we're not including that as part of the initial functionality
/** | ||
* @return The canonical string representation of this worker deployment version. | ||
*/ | ||
public String toCanonicalString() { | ||
return deploymentName + "." + buildId; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this specific class, I think just having this be the toString
instead of the generated one below would be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the current approach since the canonical format is different then the standard toString
implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, not sure there is such thing as a "standard" toString
implementation, especially for a basic dot-delimited tuple like this. Same as there isn't a standard builder expectation for such a simple class. Having said that, I don't mind if we keep them separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I looked into this when I wrote it and it seems the consensus is that toString is more for debugging than canonical representations
temporal-sdk/src/main/java/io/temporal/common/WorkerDeploymentVersion.java
Show resolved
Hide resolved
temporal-sdk/src/main/java/io/temporal/common/VersioningBehavior.java
Outdated
Show resolved
Hide resolved
* required to specify a behavior. See {@link | ||
* io.temporal.worker.WorkerOptions.Builder#setDeploymentOptions(WorkerDeploymentOptions)}. | ||
*/ | ||
VERSIONING_BEHAVIOR_UNSPECIFIED, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we represent the absence of a versioning behavior as null instead of unspecified? If we do represent it as unspecified, can you confirm that all places we return this value to the user have it as non-null unspecified when unset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's the case. (with the minor exception of the one workflow method metadata thing where it makes sense that it should be able to represent both, because it needs to distinguish un-annotated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 So long as we never give this value back to the user as null and always give it back as unspecified, works for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue tracking the client-side changes? Specifically, version info for list, describe, and scheduling, and the new versioning calls on the client. (asking at top of this file because this stuff is easier in a thread than PR comment and there's no better place)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
temporal-sdk/src/main/java/io/temporal/worker/WorkerOptions.java
Outdated
Show resolved
Hide resolved
* implementations, not interfaces. | ||
*/ | ||
@Retention(RetentionPolicy.RUNTIME) | ||
@Target(ElementType.METHOD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this annotation would be on the class not the run method. I also expect it to be a workflow/class-level setting in other SDKs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being on the workflow method is correct. Workflow implementations can contain multiple workflow methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrmm, my concern is that versioning applies to the entire workflow and all of its code, not just its primary method. But knowing that Java is a bit unique here and in every other SDK this versioning setting will apply to the workflow code as a whole and not just the entry point is probably ok, if unfortunate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Method is the workflow, a class can contain multiple unrelated workflows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method is only part of the workflow, there is lots of code that makes up a workflow. We can only hope that when they use versioning and incompatibly change a signal handler, they remember to update the versioning enum on all primary workflow methods and not just one. But I figure multi-workflow-method workflow classes are probably not that common.
We also need to add support for DynamicWorkflows, I think it can be a method on the interface like |
7fd5821
to
62d7d58
Compare
62d7d58
to
dc248a0
Compare
import io.temporal.workflow.WorkflowVersioningBehavior; | ||
import org.springframework.context.ConfigurableApplicationContext; | ||
|
||
@WorkflowImpl(workers = "mainWorker") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn Springboot taking this great annotation name for itself hehe
*/ | ||
@Experimental | ||
@Nullable | ||
public static VersioningBehavior getVersioningBehaviorForMethod( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to make this a bit more generic if we plan to add other annotations to workflow impl. methods. Since this is experimental I am fine leaving it as is.
1199b57
to
db3b822
Compare
db3b822
to
928da52
Compare
What was changed
Added worker options for the latest iteration of the worker versioning feature, as well as annotations for workflow methods to specify their behavior.
Why?
New feature!
Checklist
Closes Support New Worker Versioning API #2458
How was this tested:
Added tests
Any docs updates needed?