Skip to content

ZOOKEEPER-2488: Synchronized access to shuttingDownLE in QuorumPeer#342

Closed
bitgaoshu wants to merge 2 commits intoapache:masterfrom
bitgaoshu:ZOOKEEPER-2488
Closed

ZOOKEEPER-2488: Synchronized access to shuttingDownLE in QuorumPeer#342
bitgaoshu wants to merge 2 commits intoapache:masterfrom
bitgaoshu:ZOOKEEPER-2488

Conversation

@bitgaoshu
Copy link
Copy Markdown
Contributor

No description provided.

private void electionAndSetCurVote() {
reconfigFlagClear();
if (shuttingDownLE) {
startLeaderElection();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come we don't need to set shuttingDownLE back to false here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, sorry. it's my negligence. i will fix

Copy link
Copy Markdown

@DanBenediktson DanBenediktson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't obvious to me why it's not necessary to clear the flag in this path, which the code used to do. Tried digging around in the code to figure out what the flag was used for (I've basically never looked at the LE code before), and I'm still not certain either way whether it's actually necessary or not, but I'd be kind of surprised if it isn't.

Copy link
Copy Markdown
Contributor

@hanm hanm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

volatile alone will not work here because the state we are trying to protected here is not just the shuttingDownLE itself but the state of the entire QuorumPeer. An example of an existing code piece that's properly protected:

public synchronized void restartLeaderElection(QuorumVerifier qvOLD, QuorumVerifier qvNEW){

@hanm
Copy link
Copy Markdown
Contributor

hanm commented Aug 24, 2017

}
}

private void electionAndSetCurVote() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO,If you really want to refactor the code block(Line1137-Line1149) ,throws Exception may be better than catching exception in this function!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enen, i think the exception can be appropriately handled in this function, why need to throw the exception to caller? :) thanks

@bitgaoshu bitgaoshu closed this Aug 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants