You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2020/10/22 00:28:58 UTC
[GitHub] [bookkeeper] karanmehta93 opened a new pull request #2450: ZookeeperClient should retry instantly on zk session expiry
karanmehta93 opened a new pull request #2450:
URL: https://github.com/apache/bookkeeper/pull/2450
Descriptions of the changes in this PR:
### Motivation
Currently, we wait for `sessionTimeoutMs` before initiating a new zk cnxn. Session expiry is recevied only
when zk quprum is in a healthy state and can acquire a quorum to expire a session.
All other connection loss retries happen at zk client library transparently.
There is not much benefit waiting at BK client level.
### Changes
Updated the values for backoff and maxbackoff to 0 seconds.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] karanmehta93 commented on a change in pull request #2450: ZookeeperClient should retry instantly on zk session expiry
Posted by GitBox <gi...@apache.org>.
karanmehta93 commented on a change in pull request #2450:
URL: https://github.com/apache/bookkeeper/pull/2450#discussion_r512423320
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/zookeeper/ZooKeeperClient.java
##########
@@ -229,7 +229,7 @@ public ZooKeeperClient build() throws IOException, KeeperException, InterruptedE
if (null == connectRetryPolicy) {
connectRetryPolicy =
- new BoundExponentialBackoffRetryPolicy(sessionTimeoutMs, sessionTimeoutMs, Integer.MAX_VALUE);
+ new BoundExponentialBackoffRetryPolicy(0, 0, Integer.MAX_VALUE);
Review comment:
I will add it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] jiazhai merged pull request #2450: ZookeeperClient should retry instantly on zk session expiry
Posted by GitBox <gi...@apache.org>.
jiazhai merged pull request #2450:
URL: https://github.com/apache/bookkeeper/pull/2450
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2450: ZookeeperClient should retry instantly on zk session expiry
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2450:
URL: https://github.com/apache/bookkeeper/pull/2450#discussion_r511591882
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/zookeeper/ZooKeeperClient.java
##########
@@ -229,7 +229,7 @@ public ZooKeeperClient build() throws IOException, KeeperException, InterruptedE
if (null == connectRetryPolicy) {
connectRetryPolicy =
- new BoundExponentialBackoffRetryPolicy(sessionTimeoutMs, sessionTimeoutMs, Integer.MAX_VALUE);
+ new BoundExponentialBackoffRetryPolicy(0, 0, Integer.MAX_VALUE);
Review comment:
can we add a comment explaining this choice ?
this way in the future it will be easier to not fall into a question like "why are we using an hardcoded 0 value" ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] karanmehta93 commented on pull request #2450: ZookeeperClient should retry instantly on zk session expiry
Posted by GitBox <gi...@apache.org>.
karanmehta93 commented on pull request #2450:
URL: https://github.com/apache/bookkeeper/pull/2450#issuecomment-716989934
I am not sure if distributedlog module would want that or not. Based on our current experience with BK code, these hardcoded options would work just fine. Adding a new config parameter will be adding complexity to something not probably won't be tuned much anyways.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] karanmehta93 commented on a change in pull request #2450: ZookeeperClient should retry instantly on zk session expiry
Posted by GitBox <gi...@apache.org>.
karanmehta93 commented on a change in pull request #2450:
URL: https://github.com/apache/bookkeeper/pull/2450#discussion_r513966600
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/zookeeper/ZooKeeperClient.java
##########
@@ -229,7 +229,7 @@ public ZooKeeperClient build() throws IOException, KeeperException, InterruptedE
if (null == connectRetryPolicy) {
connectRetryPolicy =
- new BoundExponentialBackoffRetryPolicy(sessionTimeoutMs, sessionTimeoutMs, Integer.MAX_VALUE);
+ new BoundExponentialBackoffRetryPolicy(0, 0, Integer.MAX_VALUE);
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2450: ZookeeperClient should retry instantly on zk session expiry
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2450:
URL: https://github.com/apache/bookkeeper/pull/2450#issuecomment-717265204
I agree with @karanmehta93
there is no need for an additional tunable parameter.
Regarding DLog I am not sure.
"Don't fix things that are not broken":
If there is no user that is using that is reporting problems and @karanmehta93 has no experience with that code we could break it instead of fixing it
@diegosalvi is using DLog in EmailSuccess, probably he can have more context
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org