-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Require project-id for waitForPersistentTasksCondition #124180
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
Require project-id for waitForPersistentTasksCondition #124180
Conversation
The method should be called with an explicit project-id to access persistent tasks from the right project. This PR does that. The callsites are updated by using the default project-id for the timebeing. They work for single project deployments but should eventually be updated for multi-project setup.
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| if (persistentTasksCustomMetadata == null) { | ||
| return 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.
I added similar null check in most of the call sites. It looks correct to me since all usages are either waiting for all tasks to be removed or not on certain state. Additionally, the same null check is already in place for TransportStopTransformAction.
I think it's likely a noop for single project setup since the method is called only after the tasks have started. Having it feels more future proof for multi-project where project might be concurrently removed. It might be that the project deletion is orderly so that a project won't be removed until all tasks have cleared out. In that case, this check will be a noop. Overall I feel it is safer to have it in any case.
| * @param timeout a timeout for waiting | ||
| * @param listener the callback listener | ||
| */ | ||
| public void waitForPersistentTasksCondition( |
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.
Is there a reason you didn't also update the (overloaded) waitForPersistentTasksCondition method above? I think it makes sense to do both within this PR.
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 other method is not an overload, it is waitForPersistentTaskCondition, note the singular Task. I didn't touch that because you handled it in #124000?
nielsbauman
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
|
@elasticmachine update branch |
The method should be called with an explicit project-id to access persistent tasks from the right project. This PR does that. The callsites are updated by using the default project-id for the timebeing. They work for single project deployments but should eventually be updated for multi-project setup. Relates: ES-11039
The method should be called with an explicit project-id to access persistent tasks from the right project. This PR does that. The callsites are updated by using the default project-id for the timebeing. They work for single project deployments but should eventually be updated for multi-project setup. Relates: ES-11039
The method should be called with an explicit project-id to access persistent tasks from the right project. This PR does that. The callsites are updated by using the default project-id for the timebeing. They work for single project deployments but should eventually be updated for multi-project setup.
Relates: ES-11039