-
Notifications
You must be signed in to change notification settings - Fork 136
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
Apply lock at Coordinator class level #979
Conversation
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Outdated
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Outdated
Show resolved
Hide resolved
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.
great find!! 🙌
f92c00a
@@ -3230,6 +3230,7 @@ void testOnSessionExpired(boolean handleNewSession) throws DatastreamException, | |||
Assert.assertTrue(t != null && t.isAlive()); | |||
Assert.assertTrue(PollUtils.poll(() -> instance1.getIsLeader().getAsBoolean(), 100, 30000)); | |||
} | |||
instance1._coordinatorEventThreadExiting = false; |
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.
interesting that we have to set this explicitly 🤔
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 test do zkExpiry and then handleNewSession and then call stop. So during zkExpiry, we go from false -> true, and during stop if _coordinatorEventThreadExiting
stays false, then wait does not occur and creates deadlock.
As part of #975 where we used
it was giving lock on the
CoordinatorEventProcessor
instance. However, we need a lock on theCoordinator
class, hence the notifyAll was not working as expected.This PR fixes the above regression by introducing