-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
KAFKA-6879; Invoke session init callbacks outside lock to avoid deadlock #4977
Conversation
cc @junrao I implemented the fix we discussed offline. We are still ensured that the expiration event is received prior to establishing the new session, which means controller resignation must happen before any further events. I think this fences off any events which might be added prior to session initialization, but let me know if I missed something. |
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.
@hachikuji : Thanks for the patch. LGTM. A couple of minor comments below. Also, we want expiryScheduler to be single threaded.
expireEvent.waitUntilProcessed() | ||
|
||
// Block initialization of the new session until the expiration event is being handled to | ||
// ensure it cannot be associated with the wrong session |
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 mainly to ensure that all pending events in the queue are processed before we create a new ZK session.
info("Session expired.") | ||
|
||
// Initialization callbacks are invoked outside of the lock to avoid deadlock potential | ||
// since callbacks may acquire their own locks and add new requests |
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.
"since callbacks may acquire their own locks and add new requests" => since the completion of beforeInitializingSession() in the state change handler may require Zookeeper requests ?
} | ||
|
||
/** | ||
* reinitialize method to use in unit tests | ||
*/ | ||
private[zookeeper] def reinitialize(): Unit = { | ||
private[zookeeper] def forceReinitialize(): Unit = { |
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.
Shall we add a comment explaining how this differs from reinitialize
?
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, just a couple of minor comments.
ZooDefs.Ids.OPEN_ACL_UNSAFE.asScala, CreateMode.PERSISTENT)) | ||
} finally { | ||
latch.countDown() | ||
} |
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.
Nit: we can drop the braces from try and finally. Unless you think they make the code clearer?
} | ||
|
||
reinitializeThread.start() | ||
Thread.sleep(100) |
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.
Shall we explain why we need the sleep
?
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.
@hachikuji : Thanks for the updated patch. LGTM
…oller deadlock (#4977) Fixes a deadlock between the controller's beforeInitializingSession callback which holds the zookeeper client initialization lock while awaiting completion of an asynchronous event which itself depends on the same lock. Also catch and log callback exceptions to ensure the ZooKeeper reconnection takes place. Finally, configure KafkaScheduler in ZooKeeperClient to have at least 1 thread. Added tests that fail or hang without the changes in this PR. Reviewers: Jun Rao <junrao@gmail.com>, Ismael Juma <ismael@juma.me.uk>
Merged to trunk and 1.1. |
…oller deadlock (apache#4977) Fixes a deadlock between the controller's beforeInitializingSession callback which holds the zookeeper client initialization lock while awaiting completion of an asynchronous event which itself depends on the same lock. Also catch and log callback exceptions to ensure the ZooKeeper reconnection takes place. Finally, configure KafkaScheduler in ZooKeeperClient to have at least 1 thread. Added tests that fail or hang without the changes in this PR. Reviewers: Jun Rao <junrao@gmail.com>, Ismael Juma <ismael@juma.me.uk>
Fixes a deadlock between the controller's
beforeInitializingSession
callback which holds the zookeeper client initialization lock while awaiting completion of an asynchronous event which itself depends on the same lock.Committer Checklist (excluded from commit message)