-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix issue with CuratorLoadQueuePeon shutting down executors it does not own #8140
fix issue with CuratorLoadQueuePeon shutting down executors it does not own #8140
Conversation
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.
Thanks for the quick fix!
@@ -335,8 +335,6 @@ public void stop() | |||
|
|||
queuedSize.set(0L); | |||
failedAssignCount.set(0); | |||
processingExecutor.shutdown(); |
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 we shut these down somewhere? (Maybe tie LoadQueueTaskMaster
to lifecycle and shutdown the execs there?)
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.
That seems reasonable, 👍
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.
Used the factory
passed into the method in CliCoordinator
to create the executors, which itself appears to be under lifecycle.
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.
Ah, that won't work...
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.
Ah, that won't work...
Fixed to use ExecutorServices.manageLifecycle
to tie the executors to the main service lifecycle.
…ot own (apache#8140) * fix issue with CuratorLoadQueuePeon shutting down executors it does not own * use lifecycled executors * maybe this
…ot own (apache#8140) * fix issue with CuratorLoadQueuePeon shutting down executors it does not own * use lifecycled executors * maybe this
…ot own (apache#8140) (apache#8151) * fix issue with CuratorLoadQueuePeon shutting down executors it does not own * use lifecycled executors * maybe this
Fixes #8137.
Description
#7088 implemented parallel loading for
CuratorLoadQueuePeon
, but is incorrectly shutting down the peon executor and callback executor that is shared by all peons in the stop method of any peon. This means that the coordinator will operate correctly until a server disappears for any reason, which will then lead to an exception of the form:as described in #8137.
This PR fixes the issue by not shutting down the executors.
This PR has: