Skip to content
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

Use default consistency level for visibility DeleteWorkflowExecution #3132

Merged
merged 2 commits into from
Jul 26, 2022

Conversation

dhruva-groww
Copy link
Contributor

@dhruva-groww dhruva-groww commented Jul 22, 2022

What changed?
Use default consistency level for visibility DeleteWorkflowExecution.

Why?
https://community.temporal.io/t/getting-error-on-deleting-workflows-while-using-astradb-serverless-with-temporal/5356

How did you test it?
Existing tests.

Potential risks
No risks.

Is hotfix candidate?
No.

@dhruva-groww dhruva-groww requested a review from a team as a code owner July 22, 2022 12:28
@CLAassistant
Copy link

CLAassistant commented Jul 22, 2022

CLA assistant check
All committers have signed the CLA.

@yiminc yiminc requested a review from alexshtin July 22, 2022 17:00
@@ -143,7 +143,7 @@ func NewVisibilityStore(

return &visibilityStore{
session: session,
lowConslevel: gocql.One,
lowConslevel: gocql.Consistency(cfg.Consistency.GetConsistency()),
Copy link
Member

Choose a reason for hiding this comment

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

@dhruva-groww, is there specific issue that you are encountered with this?
The consistency level One is used intentionally as lowConslevel for read operations. I'm not sure why the Delete operation is also using this lowConslevel, is it the one that is causing trouble?
cc @alexshtin @rodrigozhou

Copy link
Contributor Author

@dhruva-groww dhruva-groww Jul 22, 2022

Choose a reason for hiding this comment

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

Yes. I assumed the writes that occur should occur as per the configuration provided in config (default as local_quorum) under visibility store.
I discovered this issue while integrating with DataStax which does not allow ONE as a consistency level while writing (as a good practice as it may cause data loss). I had raised it here with logs and exceptions
https://community.temporal.io/t/getting-error-on-deleting-workflows-while-using-astradb-serverless-with-temporal/5356

Copy link
Member

Choose a reason for hiding this comment

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

Instead of changing the lowConslevel here, could you change the DeleteWorkflow to not use consistency One?

if err := query.Consistency(v.lowConslevel).Exec(); err != nil {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On it

@yiminc yiminc changed the title changing visibility store consistency from goCql.One to as provided i… Use default consistency level visibility delete request Jul 23, 2022
@yiminc
Copy link
Member

yiminc commented Jul 25, 2022

@dhruva-groww please sign the CLA to land this PR.

@dhruva-groww
Copy link
Contributor Author

dhruva-groww commented Jul 25, 2022 via email

@alexshtin alexshtin changed the title Use default consistency level visibility delete request Use default consistency level for visibility DeleteWorkflowExecution Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants