Added minor adjustments in the shadow status options #1359
Conversation
agautam478
requested review from
Groxx,
shijiesheng,
jakobht,
3vilhamster,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -183,7 +181,7 @@ func ToWorkflowStatus(statusString string) (WorkflowStatus, error) { | |||
| status := WorkflowStatus(strings.ToUpper(statusString)) | |||
| switch status { | |||
| case WorkflowStatusOpen, WorkflowStatusClosed, WorkflowStatusCompleted, | |||
| WorkflowStatusFailed, WorkflowStatusCanceled, WorkflowStatusTerminated, | |||
| WorkflowStatusFailed, WorkflowStatusCanceled, | |||
There was a problem hiding this comment.
you can still leave it here in the mapping function.
There was a problem hiding this comment.
I will readd it later. removed it to be sure that the status can not be used in any situation at all.
internal/workflow_shadower.go
Outdated
| @@ -338,10 +338,9 @@ func (o *ShadowOptions) validateAndPopulateFields() error { | |||
| } | |||
| statuses = append(statuses, status) | |||
| } | |||
| //All the open statuses are taken by default. This list seems to not work as expected. | |||
| //TODO: verify that the status list works as expected. currently all wfs of all types get picked up. | |||
| //This filter doesn't seem to be working as expected. | |||
There was a problem hiding this comment.
please elaborate in the comment what you mean
There was a problem hiding this comment.
This filter should have only taken open and closed wfs but it returns continue as new wfs as well. not sure if this is because of some other check somewhere or the query gets formed incorrectly.
There was a problem hiding this comment.
Please elaborate on this in the comment so it will be easier to identify the problem.
Consider adding a TODO: handle ContinueAsNew as a special case.
| @@ -203,7 +203,7 @@ func (s *workflowShadowerSuite) TestShadowOptionsValidation() { | |||
| options: ShadowOptions{}, | |||
| expectErr: false, | |||
| validationFn: func(options *ShadowOptions) { | |||
| s.Equal("(CloseTime = missing)", options.WorkflowQuery) | |||
| s.Equal("(CloseTime = missing or CloseTime != missing)", options.WorkflowQuery) | |||
There was a problem hiding this comment.
is not this equivalent to empty query?
There was a problem hiding this comment.
it is but the query builder forms it this way because of open and closed statuses getting added together in the test. This portion of default status check needs a rewrite that I will be doing as a part of the shadower fix initiative.
There was a problem hiding this comment.
we do have an ALL status filter somewhere but it barely gets used. Now the problem with that one is that it picks up everything present in that defaut list of statuses.
internal/workflow_shadower.go
Outdated
| @@ -338,10 +338,9 @@ func (o *ShadowOptions) validateAndPopulateFields() error { | |||
| } | |||
| statuses = append(statuses, status) | |||
| } | |||
| //All the open statuses are taken by default. This list seems to not work as expected. | |||
| //TODO: verify that the status list works as expected. currently all wfs of all types get picked up. | |||
| //This filter doesn't seem to be working as expected. | |||
There was a problem hiding this comment.
Please elaborate on this in the comment so it will be easier to identify the problem.
Consider adding a TODO: handle ContinueAsNew as a special case.
| if len(statuses) == 0 { | ||
| statuses = []WorkflowStatus{WorkflowStatusOpen} | ||
| statuses = []WorkflowStatus{WorkflowStatusOpen, WorkflowStatusClosed} | ||
| } | ||
| queryBuilder.WorkflowStatus(statuses) |
There was a problem hiding this comment.
I guess modern DBs with good query analyzers will just drop this expression, but evaluation of this does not make any sense.
I suggest making this optional:
if len(statuses) != 0 {
queryBuilder.WorkflowStatus(statuses)
}
There was a problem hiding this comment.
will incorporate this and update PR. Okay! so there's some other bug somewhere in there which created strange behavour in the filter so leaving the above as it is. I will be scrapping this piece anyway. I need this diff as an interim fix so that I ca continue working on the qb update without interrupting the customers.
What changed?
Added minor adjustments in the shadow status options so that Terminated workflows are never replayed.
Removed the terminated workflow status from the All option as well.
Added default option for replaying only closed and open workflows and no other workflows if workflow status filter is not passed.
Why?
This is to ensure that the terminated workflows are not replayed. We also have bugs associated with continue as new workflows that need to be fixed before those wfs can be replayed.
How did you test it?
test cases.
Potential risks
shadower accuracy may get affected.