-
Notifications
You must be signed in to change notification settings - Fork 152
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
Activity options merging logic is not correct #2042
Comments
Ignoring backwards compatibility for a moment
I would disagree that is intuitive or logical. Before reading the docs I would not expect all the options to be merged in order. Specifically I would not expect the default activity options to be merged with the specific activity options. If I pass a specific option it is because I don't want the default, I would not expect the default to still get merged if I passed a more specific option. Looking at the docs for setActivityOptions supports this. |
I see two separate questions here:
I personally think we want merging as it allows overriding either specific values as well as all values. The precedence outlined above is the only one that makes sense to me. I see a specific exception for testing where the options sometimes need to be forcefully replaced through WorkflowTestEnvironment.
The doc explicitly says about merging:
|
I disagree, I find the outlined approach unintuitive for the reasons I state above , specifically merging defaultActivityOptions and activityOptions, and it disagrees with our documentation. |
Would you specify the currently implemented rules for the 6 sources of options and what is the logic behind them? |
I am specifically talking about: WorkflowImplementationOptions#defaultActivityOptions If I set a default value for something and a specific value I would not expect them to be merged, the documentation does not say they will be merged. The documentation for
|
To clarify my opinion is default options should not be used if a more specific option for an activity type is specified. A default is a preselect option if no other is present, not a base to build more specific options form . This is
The only time the default options should be considered is if no options are provided so , ignoring
Otherwise, If activity options are provides we should use the activity options, again ignoring
Edit: I believe this is the documented and intended behaviour, but the documentation and code is spread out enough that it is not trivial obvious which we should fix. |
Expected Behavior
There are six potential sources for activity options:
When all these options are specified for the given activity type, the logical behavior is to merge them in the same order as specified above.
Actual Behavior
The current logic is split between:
sdk-java/temporal-sdk/src/main/java/io/temporal/internal/sync/WorkflowInternal.java
Line 303 in 8af4a26
and
sdk-java/temporal-sdk/src/main/java/io/temporal/internal/sync/ActivityInvocationHandler.java
Line 70 in 16755a1
The short description is:
options = options argument passed to [WorkflowInternal#newActivityStub] ? WorkflowImplementationOptions#defaultActivityOptions // not that they are not merged if the argument is present.
WorkflowImplementationOptions#activityOptions map is overridden by options found in activityMethodOptions map argument passed to WorkflowInternal#newActivityStub
options (from step 1) are overridden by a value (with the key equal to the activity type) from the map generated by step (2)
I believe that we should fix the merging logic to match the intuitive behavior explained in the "Expected Behavior" section.
The text was updated successfully, but these errors were encountered: