-
Notifications
You must be signed in to change notification settings - Fork 320
Use wait_for_completion:False instead of relying on timeouts in ForceMerge #2016
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 wait_for_completion:False instead of relying on timeouts in ForceMerge #2016
Conversation
fressi-elastic
left a comment
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.
Please have a look to my comments. Overall it LGTM
fressi-elastic
left a comment
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.
Resulting unit test failures looks related. Please fix it.
gbanasiak
left a comment
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.
LGTM.
When run with wait_for_completion=true, force merge returns task ID. This task ID could be used to filter the list of force merge tasks in case there's parallel unrelated force merge running in the cluster. WDYT about adding it as part of this PR?
Relying on a timeout in the force merge runner can result in other errors - e.g if a load balancer kills a connection, typically we would see an SSL related error instead. This change changes polling method to use wait_for_completion:False instead and then polls via the task API.