Populate tasks in internal workflowTask and activityTask entities for empty polls #1416
Conversation
shijiesheng
requested review from
3vilhamster,
Groxx,
demirkayaender,
dkrotx,
jakobht and
taylanisikdemir
as code owners
internal/internal_task_handlers.go
Outdated
| // workflowTask wraps a decision task. | ||
| workflowTask struct { | ||
| task *s.PollForDecisionTaskResponse | ||
| autoConfigHint *s.AutoConfigHint |
There was a problem hiding this comment.
why can not we reuse the autoConfigHint inside task(PollForDecisionTaskResponse) here? I'm assuming task != nil will mess up the logic somewhere else
There was a problem hiding this comment.
I haven't tried. But the field is widely used in many different places. Breaking the Nilness assumption might mess it up.
There was a problem hiding this comment.
I don't think we need the new field. It's a redundant information and requires each instantiation of workflowTask/activityTask to fill it based on the response.
Instead expose a helper function such as:
func (wt *workflowTask) GetAutoConfigHint() *s.AutoConfigHint {
if wt.task == nil {
return nil
}
return wt.task.GetAutoConfigHint()
Less code to write and maintain.
There was a problem hiding this comment.
yeah, I checked carefully. We can populate this task field for empty polls. Thus I've changed the title as well.
shijiesheng
changed the title
| common.PtrOf(int64(1000)), | ||
| }, | ||
| } | ||
| for _, tt := range []struct { |
There was a problem hiding this comment.
let's also add nil cases
There was a problem hiding this comment.
It's actually not possible for client to return nil, nil for both response and error so I'll skip the test
shijiesheng
force-pushed
the
empty_polls
branch
from
e16fdbc to
5f75c34
Compare
… empty polls (cadence-workflow#1416) What changed? ensure task field is populated in workflowTask and activityTask for empty polls. Why? Task response even with empty task contains information like AutoConfigHint, which is needed for poller auto scaler How did you test it? Unit Test
…ties for empty polls (cadence-workflow#1416)" This reverts commit d9dcddc.
…ties for empty polls (#1416)" (#1424) This reverts commit d9dcddc. What changed? A pure revert of d9dcddc#diff-caac3c5868c1fd134487cacc53e459b7375722e23c6aba3e9407521feba00007 Why? Nilness check is no longer enough after the commit One alternative is to add additional emptiness check. This makes the client code error prone. Thus, a pure revert is desired. How did you test it? Unit Test
3 participants
What changed?
ensure task field is populated in workflowTask and activityTask for empty polls.
Why?
Task response even with empty task contains information like AutoConfigHint, which is needed for poller auto scaler
How did you test it?
Unit Test
Potential risks