IWF-137: Allow to use separate persistency loading policy for waitUntil #448
Conversation
…bjectsLoadingPolicy
…d executeApiSearchAttributesLoadingPolicy; with fallback to searchAttributesLoadingPolicy
samuel27m
requested review from
longquanzheng,
mixydubs,
stevo89519 and
lwolczynski
| if stateOptions.HasDataObjectsLoadingPolicy() { | ||
| return stateOptions.DataObjectsLoadingPolicy | ||
| } | ||
|
|
||
| if stateOptions.HasDataAttributesLoadingPolicy() { | ||
| return stateOptions.DataAttributesLoadingPolicy | ||
| } | ||
|
|
||
| return stateOptions.WaitUntilApiDataAttributesLoadingPolicy | ||
| } |
There was a problem hiding this comment.
I think the expected behavior is to prefer more specific to override less specific -- waitUntilDALoadingPolicy will override daLoadingPolicy, then DataObjectsLoadingPolicy, then nil
There was a problem hiding this comment.
When I did this I tried copying from the existing fallback from DataAttributesLoadingPolicy to HasDataObjectsLoadingPolicy:
if stateOptions.HasDataObjectsLoadingPolicy() {
return stateOptions.DataObjectsLoadingPolicy
}
return stateOptions.DataAttributesLoadingPolicy
However, you're absolutely right, more specific should override less specific, I'll make the changes.
I will also implement a integ test that ensures that the more specific policy is the one being picked up if they're both specified.
There was a problem hiding this comment.
Done now ✅
More specific policies should override less specific ones
| /** | ||
| * This test has four states | ||
| * | ||
| * State1: Creates all Data Attributes keys that will be used in this test |
There was a problem hiding this comment.
Can you provide details on what does WaitUntil and Execute do? F.g. State1 doesn't do anything in WaitUntil.
| ) | ||
|
|
||
| /** | ||
| * This test has four states |
There was a problem hiding this comment.
Nice! I planned to add documentation to all the exsting integ test workflow, and wondering what is the format. I think it's a great idea that just put it at the beginning.
Nit: This test has four states -> This test workflow has four states, using REST controller to implement the workflow directly.
There was a problem hiding this comment.
Will make the change! Thanks!
Do you think that is best for the comment to be at the start of the test routers file (where it currently is) or at the start of the actual test file (i.e. wf_state_options_data_attributes_loading_test.go)?
| * - da_other_key | ||
| * State2: | ||
| * - Declares State Options containing WaitUntilApiDataAttributesLoadingPolicy | ||
| * - Asserts that (ApiV1WorkflowStateStart) WaitUntil method will load with expected Data Attributes |
There was a problem hiding this comment.
also better to clarirfy where is the Asserts: in waitUntil or execute
There was a problem hiding this comment.
I did specify where the Asserts is: - Asserts that (ApiV1WorkflowStateStart) **WaitUntil** method will load with expected Data Attributes
But this probably means that the comment is not very readable. I'll make changes so that this reads better!
There was a problem hiding this comment.
I made changes to the comment structure so that it is more readable. Let me know what you think, we can also brainstorm about it on a call if you prefer! 👍
Makefile
Outdated
| @@ -138,7 +138,7 @@ iwf-server: | |||
|
|
|||
| .PHONY: bins release clean | |||
|
|
|||
| idl-code-gen: #generate/refresh go clent code for idl, do this after update the idl file | |||
| idl-code-gen: #generate/refresh go client code for idl, do this after update the idl file | |||
There was a problem hiding this comment.
also maybe fix the wording, "client code" may be confusing with our sdk. I would just say "go code for idl(API schema".
…ty over less specific ones
…ing policies take priority over less specific one
…g policies take priority over less specific one
| @@ -45,14 +45,62 @@ func GetDecideApiRetryPolicy(stateOptions *iwfidl.WorkflowStateOptions) *iwfidl. | |||
| return stateOptions.ExecuteApiRetryPolicy | |||
| } | |||
|
|
|||
| func GetDataObjectsLoadingPolicy(stateOptions *iwfidl.WorkflowStateOptions) *iwfidl.PersistenceLoadingPolicy { | |||
| func GetWaitUntilApiDataObjectsLoadingPolicy(stateOptions *iwfidl.WorkflowStateOptions) *iwfidl.PersistenceLoadingPolicy { | |||
There was a problem hiding this comment.
DataObject is the old name, let's rename it to the new one if possible (there are more we need to do in the repo)
There was a problem hiding this comment.
I'll create a new issue for this 👍
Description
Checklist
Related Issue
Closes #387