IWF-163: Add retry policy to QueryWorkflow calls #491
Conversation
lwolczynski
force-pushed
the
jira/lwolczynski/IWF-163
branch
from
68fa7a7
to
7feffc2
Compare
cmd/server/iwf/iwf.go
Outdated
| retryPolicy := temporalapi.QueryWorkflowFailedRetryPolicy{ | ||
| InitialIntervalSeconds: config.GetInitialIntervalSecondsWithDefault(), | ||
| MaximumAttempts: config.GetMaximumAttemptsWithDefault(), | ||
| } | ||
| unifiedClient = temporalapi.NewTemporalClient(temporalClient, config.Interpreter.Temporal.Namespace, converter.GetDefaultDataConverter(), false, retryPolicy) |
There was a problem hiding this comment.
If you have config.GetInitialIntervalSecondsWithDefault(), then the expectation is that the code that need to use the value will call it to get the value with default, instead of passing into here.
So it will be temporalapi.NewTemporalClient(temporalClient, config.Interpreter.Temporal.Namespace, converter.GetDefaultDataConverter(), false, config.ApiConfig.QueryRetryPolicy)
Also consider using pointers to make this optional and load the default values in client.go.
Basically the config can be defined it as a pointer instead of struct. So that internally, we just need to pass a nil into this, which should let it use the default value.
service/client/cadence/client.go
Outdated
| qres, err = queryWorkflowWithStrongConsistency(t, ctx, workflowID, runID, queryType, args) | ||
| if err != nil { | ||
| if t.isQueryFailedError(err) { | ||
| if attempt == t.queryWorkflowFailedRetryPolicy.MaximumAttempts { |
There was a problem hiding this comment.
Nit: I don't think we need another check here, since it'a already checked by the for loop condition?
Although we will have to check err != nil again outside of the loop
There was a problem hiding this comment.
Boils down to the same thing, but it might be a little cleaner. Done
lwolczynski
force-pushed
the
jira/lwolczynski/IWF-163
branch
from
b42805b
to
fc4f2d6
Compare
service/client/temporal/client.go
Outdated
| ) uclient.UnifiedClient { | ||
| var rp QueryWorkflowFailedRetryPolicy | ||
|
|
||
| if retryPolicy.InitialIntervalSeconds == 0 { |
There was a problem hiding this comment.
If I'm not mistaken, not providing Config.ApiConfig.QueryWorkflowFailedRetryPolicy in yaml file will make the default int value show (which is 0). In other words, we shouldn't need a nil check here
Description
Checklist
Related Issue
Closes #437