Support None to not load any data for the persistence loading policy …

[zklgame]

Support attributes loading None in RPC with code refactor

[zklgame]

@longquanzheng The s.client.QueryWorkflow seems to be needed whatever the loading policies are since it gets other info like worker url, run id, timestamp.

[codecov]

Codecov Report

Merging #320 (85a48f6) into main (c01cd95) will increase coverage by 0.28%.
The diff coverage is 94.66%.

@@            Coverage Diff             @@
##             main     #320      +/-   ##
==========================================
+ Coverage   71.62%   71.90%   +0.28%     
==========================================
  Files          54       54              
  Lines        4423     4457      +34     
==========================================
+ Hits         3168     3205      +37     
+ Misses       1019     1017       -2     
+ Partials      236      235       -1     

Files Changed	Coverage Δ
service/interpreter/persistence.go	92.57% <92.10%> (-1.30%)	⬇️
service/api/service.go	77.46% <97.29%> (+2.41%)	⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[longquanzheng]

@longquanzheng The s.client.QueryWorkflow seems to be needed whatever the loading policies are since it gets other info like worker url, run id, timestamp.

ahh right. Maybe you can fix/improve this in the same PR(or separate if you prefer): https://github.com/indeedeng/iwf/blob/main/service/api/service.go#L112
We could always put this URL into memo/caching is not being requested for starting workflow.
So that this RPC can try to use DescribeWF API to load workerURL and runId/timestamp when loading policy is none. (note changing the startWF code will only affect new workflow so you need to think about backward compatibility)

Because avoiding query is a very important optimization here. Query is very expensive. Otherwise none is useless for rpc.

[longquanzheng]

For testing, if we are able to optimize/improve it by avoid calling query workflow, I wonder if we can assert it in the test to make sure the query API is not being called. (we may need to think about some "hack" just for testing)

[zklgame]

@longquanzheng The s.client.QueryWorkflow seems to be needed whatever the loading policies are since it gets other info like worker url, run id, timestamp.

ahh right. Maybe you can fix/improve this in the same PR(or separate if you prefer): https://github.com/indeedeng/iwf/blob/main/service/api/service.go#L112 We could always put this URL into memo/caching is not being requested for starting workflow. So that this RPC can try to use DescribeWF API to load workerURL and runId/timestamp when loading policy is none. (note changing the startWF code will only affect new workflow so you need to think about backward compatibility)

Because avoiding query is a very important optimization here. Query is very expensive. Otherwise none is useless for rpc.

Sounds like better making it in another PR since the new PR will include quite work:

1. get the workUrl, runId, timestamp from DescribeWorkflow for Temporal
2. support backward compatibility
3. test the calling of the query API

[longquanzheng]

As our/golang convention, the internal usage of a method/function should be kept as "private". In golang, the any var/method/func start with lower case is package private loadSearchAttributes.
Also there is already another method LoadSearchAttributes, you may want to rename it to internalLoadSearchAttributes to be more clear.

[longquanzheng]

Or maybe loadSearchAttributesByRequestedKeys

[longquanzheng]

I think it's a bit wastful to call am.GetAllDataObjects() before LoadDataAttributes. Because GetAllDataObjects will allocate a new slice/array to hold all the data attributes. Maybe LoadDataAttributes can just access to all the DAs directly

[longquanzheng]

we need to return error for this case. this will be supported in #305

[zklgame]

Close this one and to make a new one since it has been several weeks.