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
Remove TTL from open_executions Cassandra visibility table #1456
Remove TTL from open_executions Cassandra visibility table #1456
Conversation
if _, notFound := err.(*serviceerror.NotFound); !notFound { | ||
return err | ||
} |
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.
the task processing logic will treat not found as 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.
This is why it is not returned.
retentionSeconds = int64(timestamp.DurationFromDays(namespaceEntry.GetRetentionDays(workflowID)).Seconds()) | ||
var retention *time.Duration | ||
namespace := defaultNamespace | ||
recordWorkflowClose := true |
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.
should the sampling logic be handled by the visibility sampling store?
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.
I was also surprised to see it there but as soon as I am going to replace it with throttling soon, will just leave it there for now.
What changed?
Remove TTL from
open_executions
Cassandra visibility table.Why?
Previously TTL was set to workflow timeout as an extra protection in case of close workflow execution task lost. Also there was a bug in this protection which set this TTL to 1 day in case of
nil
(unlimited timeout). This protection is not needed with new visibility queue.How did you test it?
Current tests.
Potential risks
No risks.
Is hotfix candidate?
Yes.