Added minor adjustments in the shadow status options #1359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Open

agautam478 wants to merge 5 commits into cadence-workflow:master

from agautam478:status_fix

Open

Added minor adjustments in the shadow status options #1359

agautam478 wants to merge 5 commits into cadence-workflow:master from agautam478:status_fix

Conversation

@agautam478

Copy link

Contributor

@agautam478 agautam478 commented Jul 25, 2024

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.

@agautam478


 Added minor adjustments in the shadow status options

164508f

@agautam478 agautam478 requested review from 3vilhamster, Groxx, demirkayaender, dkrotx, jakobht, shijiesheng and taylanisikdemir as code owners

July 25, 2024 17:46

@codecov

Copy link

codecov bot commented Jul 25, 2024 •

edited

Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.48%. Comparing base (ed58224) to head (75c4f1e).

Additional details and impacted files

Files	Coverage Δ
internal/query_builder.go	`87.69% <ø> (ø)`
internal/workflow_shadower.go	`77.46% <100.00%> (ø)`

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed58224...75c4f1e. Read the comment docs.

taylanisikdemir

taylanisikdemir reviewed

Jul 25, 2024

View reviewed changes

internal/query_builder.go

switch status {

case WorkflowStatusOpen, WorkflowStatusClosed, WorkflowStatusCompleted,

WorkflowStatusFailed, WorkflowStatusCanceled,WorkflowStatusTerminated,

WorkflowStatusFailed, WorkflowStatusCanceled,

Copy link

Member

@taylanisikdemir taylanisikdemir Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can still leave it here in the mapping function.

Copy link

Contributor Author

@agautam478 agautam478 Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

//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.

Copy link

Member

@taylanisikdemir taylanisikdemir Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please elaborate in the comment what you mean

Copy link

Contributor Author

@agautam478 agautam478 Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Contributor

@3vilhamster 3vilhamster Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

internal/workflow_shadower_test.go

expectErr: false,

validationFn: func(options *ShadowOptions) {

s.Equal("(CloseTime = missing)", options.WorkflowQuery)

s.Equal("(CloseTime = missing or CloseTime != missing)", options.WorkflowQuery)

Copy link

Member

@taylanisikdemir taylanisikdemir Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is not this equivalent to empty query?

Copy link

Contributor Author

@agautam478 agautam478 Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Contributor Author

@agautam478 agautam478 Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@agautam478 agautam478 requested a review from taylanisikdemir

July 25, 2024 19:12

3vilhamster

3vilhamster reviewed

Jul 29, 2024

View reviewed changes

internal/workflow_shadower.go Outdated

}

//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.

Copy link

Contributor

@3vilhamster 3vilhamster Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

internal/workflow_shadower.go

statuses = []WorkflowStatus{WorkflowStatusOpen}

statuses = []WorkflowStatus{WorkflowStatusOpen, WorkflowStatusClosed}

}

queryBuilder.WorkflowStatus(statuses)

Copy link

Contributor

@3vilhamster 3vilhamster Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

Copy link

Contributor Author

@agautam478 agautam478 Jul 30, 2024 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@agautam478 agautam478 requested a review from 3vilhamster

July 30, 2024 17:19

agautam478 and others added 2 commits

July 30, 2024 10:20

@agautam478


 Mini fix

3f2c2fc

@agautam478


 Merge branch 'master' into status_fix

41c8e3e

abhishekj720

abhishekj720 approved these changes

Aug 1, 2024

View reviewed changes

Copy link

@abhishekj720 abhishekj720 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
For future me/others, during the shadower fix initiative, default status check would be rewritten.

@agautam478


 Merge branch 'master' into status_fix

aec2f51

@agautam478 agautam478 requested a review from abhishekj720

August 21, 2024 20:40

@agautam478


 Merge branch 'master' into status_fix

75c4f1e

Reviewers

@Groxx Groxx Awaiting requested review from Groxx Groxx is a code owner

@shijiesheng shijiesheng Awaiting requested review from shijiesheng shijiesheng is a code owner

@jakobht jakobht Awaiting requested review from jakobht jakobht is a code owner

@dkrotx dkrotx Awaiting requested review from dkrotx dkrotx is a code owner

@demirkayaender demirkayaender Awaiting requested review from demirkayaender demirkayaender is a code owner

@taylanisikdemir taylanisikdemir Awaiting requested review from taylanisikdemir taylanisikdemir is a code owner

@3vilhamster 3vilhamster Awaiting requested review from 3vilhamster 3vilhamster is a code owner

@abhishekj720 abhishekj720 Awaiting requested review from abhishekj720

Labels

None yet

4 participants

@agautam478 @taylanisikdemir @3vilhamster @abhishekj720

Added minor adjustments in the shadow status options #1359

Are you sure you want to change the base?

Added minor adjustments in the shadow status options #1359

Uh oh!

Conversation

@agautam478 agautam478 commented Jul 25, 2024

Uh oh!

codecov bot commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

@taylanisikdemir taylanisikdemir Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

@agautam478 agautam478 Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

@taylanisikdemir taylanisikdemir Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

@agautam478 agautam478 Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

@3vilhamster 3vilhamster Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

@taylanisikdemir taylanisikdemir Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

@agautam478 agautam478 Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

@agautam478 agautam478 Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

@3vilhamster 3vilhamster Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

@3vilhamster 3vilhamster Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

@agautam478 agautam478 Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@abhishekj720 abhishekj720 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Jul 25, 2024 •

edited

Loading

@agautam478 agautam478 Jul 30, 2024 •

edited

Loading