-
Notifications
You must be signed in to change notification settings - Fork 874
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
Use default consistency level for visibility DeleteWorkflowExecution #3132
Conversation
…n cassandra config
@@ -143,7 +143,7 @@ func NewVisibilityStore( | |||
|
|||
return &visibilityStore{ | |||
session: session, | |||
lowConslevel: gocql.One, | |||
lowConslevel: gocql.Consistency(cfg.Consistency.GetConsistency()), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
temporal/common/persistence/visibility/store/standard/cassandra/visibility_store.go
Line 477 in d56e1d6
if err := query.Consistency(v.lowConslevel).Exec(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On it
@dhruva-groww please sign the CLA to land this PR. |
Done
…On Tue, 26 Jul 2022 at 12:37 AM, Yimin Chen ***@***.***> wrote:
@dhruva-groww <https://github.com/dhruva-groww> please sign the CLA to
land this PR.
—
Reply to this email directly, view it on GitHub
<#3132 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOEO4C7JQNUKO773UHQL2E3VV3QX3ANCNFSM54LJWIDQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
*Disclaimer - The contents of this email message and any attachments
are
confidential and are intended solely for the addressee. The
information may
also be legally privileged. This transmission is sent
in trust, for the
sole purpose of delivery to the intended recipient.
If you have received
this transmission in error, any use, reproduction
or dissemination is
strictly prohibited. Please notify the sender
immediately by email if you
have received this email by mistake and
delete this email from your
system.*
|
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.