-
Notifications
You must be signed in to change notification settings - Fork 7.3k
ZOOKEEPER-4260: Backport ZOOKEEPER-3575 to branch-3.6 #1653
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
Conversation
Author: Jie Huang <[email protected]> Reviewers: [email protected], [email protected] Closes apache#1116 from jhuan31/ZOOKEEPER-3575 (cherry picked from commit 7c1251d)
| } | ||
|
|
||
| void writePacketNow(QuorumPacket pp, boolean flush) throws IOException { | ||
| synchronized (leaderOs) { |
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.
THREAD_SAFETY_VIOLATION: Read/Write race. Non-private method Learner.writePacketNow(...) reads without synchronization from this.leaderOs. Potentially races with write in method Learner.connectToLeader(...).
Reporting because this access may occur on a background thread.
(at-me in a reply with help or ignore)
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.
If learner.asyncSending is not set, all QuorumPacket will be sent in this writePacketNow method.
If learner.asyncSending is set, all QuorumPacket will be sent in the LearnerSender thread, which doesn't need the synchronization.
Therefore, there wouldn't be potential race.
|
Please include the latest changes done in PR #1645. |
|
@arshadmohammad I cherry-pick #1645. Some configurations are not available in branch-3.6 so I do not include them. |
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerHandler.java
Outdated
Show resolved
Hide resolved
|
Other than minor import issues, changes Looks good to me. @functioner please correct the import |
arshadmohammad
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 +1
(cherry picked from commit 7c1251d) Author: functioner <[email protected]> Author: Ayush Mantri <[email protected]> Author: Jie Huang <[email protected]> Reviewers: Mohammad Arshad <[email protected]> Closes #1653 from functioner/ZOOKEEPER-4260 and squashes the following commits: 0a19eb1 [functioner] remove useless import 5d827de [Ayush Mantri] cherry-pick ZOOKEEPER-4257 and fix conflicts 469d971 [functioner] Merge branch 'branch-3.6' of https://github.com/apache/zookeeper into ZOOKEEPER-4260 e6cb767 [Jie Huang] ZOOKEEPER-3575: Moving sending packets in Learner to a separate thread
|
Merged |
(cherry picked from commit 7c1251d)