ZOOKEEPER-3774: Close quorum socket asynchronously on the leader to a… #1301
Conversation
ztzg
left a comment
ztzg
left a comment
There was a problem hiding this comment.
LGTM (non-binding), but 1. untested, and 2. see my documentation question on the Learner side of this change.
|
@ztzg I just updated the doc. As for testing, this feature has been enabled in our production zookeepers for a couple months :) |
|
@jhuan31: Great; thanks :) |
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
Overall good.
I left two comments, PTAL
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerHandler.java
Outdated
Show resolved
Hide resolved
| if (sock != null) { | ||
| long startTime = Time.currentElapsedTime(); | ||
| sock.close(); | ||
| ServerMetrics.getMetrics().SOCKET_CLOSING_TIME.add(Time.currentElapsedTime() - startTime); |
There was a problem hiding this comment.
would it be useful to record this value even in case of failure ?
There was a problem hiding this comment.
I'm hesitating... This metric is to measure the real time for closing a socket. If it fails, say, the socket is closed already, then the elapsed time is not really the time for closing the socket.
symat
left a comment
symat
left a comment
There was a problem hiding this comment.
nice, thanks for the PR!
I added a few small comments.
Also: I am wondering if we should make this the default behaviour when quorum TLS is enabled. But I don't know if this can have any side-effect. So maybe keeping it optional is the safest way forward.
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerHandler.java
Outdated
Show resolved
Hide resolved
symat
left a comment
symat
left a comment
There was a problem hiding this comment.
thanks for your changes, I added a few more requests, please take a look.
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/LearnerHandler.java
Outdated
Show resolved
Hide resolved
…void ping being blocked by long socket closing time
symat
left a comment
symat
left a comment
There was a problem hiding this comment.
thanks for the fixes! it looks good to me now.
@eolivelli @anmolnar PTAL
|
@symat @eolivelli I've addressed requested changes. Could you please take a look? Thanks! |
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
Looks good.
I will merge tomorrow.
Thank you
…void ping being blocked by long socket closing time Author: Jie Huang <[email protected]> Reviewers: ztzg, hanm, eolivelli, symat Closes apache#1301 from jhuan31/ZOOKEEPER-3774
…void ping being blocked by long socket closing time Author: Jie Huang <[email protected]> Reviewers: ztzg, hanm, eolivelli, symat Closes apache#1301 from jhuan31/ZOOKEEPER-3774
…void ping being blocked by long socket closing time Author: Jie Huang <[email protected]> Reviewers: ztzg, hanm, eolivelli, symat Closes apache#1301 from jhuan31/ZOOKEEPER-3774
…void ping being blocked by long socket closing time Author: Jie Huang <[email protected]> Reviewers: ztzg, hanm, eolivelli, symat Closes apache#1301 from jhuan31/ZOOKEEPER-3774
5 participants
…void ping being blocked by long socket closing time