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/10 09:33:15 UTC
[GitHub] [bookkeeper] eolivelli opened a new pull request #2435: BP-41 Follow Bookie Address changes tracking
eolivelli opened a new pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435
- follow Bookie Address Changes
- add flag enableBookieAddressTracking to enable/disable this feature
This is part of BP-41
----------------------------------------------------------------
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] Ghatage edited a comment on pull request #2435: BP-41 Bookie Address changes tracking
Posted by GitBox <gi...@apache.org>.
Ghatage edited a comment on pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435#issuecomment-708508804
> If you configure bookieId in your bookie then only 4.12 clients will be able to use it.
So can there be a case where we upgrade Bookies to 4.12 but disable this option and use the older `hostname:port` pairing
But then we won't be able to use 4.12 clients?
Sorry if I am misunderstanding this!
I don't see it as a problem, just wondering if we also need to call it out and update the documentation for this in the 4.12 release.
----------------------------------------------------------------
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 #2435: BP-41 Bookie Address changes tracking
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435#issuecomment-708992769
If you simply upgrade an existing cluster from 4.9/10/11 to 4.12 everything will work well.
You can set enableBookieAddressTracking=false safely as bookies are using hostname:port as bookieId and it is not expected that this address changes.
I am setting enableBookieAddressTracking=true by default because I think this is the future for BK and also in order to have all tests working with this feature.
The case of enableBookieAddressTracking=false is to save resources on ZK, by reducing the number of watcher.
If we were on ZK 3.6.x (client + server) we could leverage "Persistent Recursive Watches", that is a single watch for an entire tree of znodes, without the need to set it again after it triggers, and this will also mitigate the impact of these new additional watches
----------------------------------------------------------------
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] Ghatage merged pull request #2435: BP-41 Bookie Address changes tracking
Posted by GitBox <gi...@apache.org>.
Ghatage merged pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435
----------------------------------------------------------------
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 #2435: BP-41 Follow Bookie Address changes tracking
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435#issuecomment-706520715
also @nicoloboschi PTAL
----------------------------------------------------------------
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] Ghatage commented on a change in pull request #2435: BP-41 Bookie Address changes tracking
Posted by GitBox <gi...@apache.org>.
Ghatage commented on a change in pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435#discussion_r504778444
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java
##########
@@ -1764,6 +1767,27 @@ public ClientConfiguration setDelayEnsembleChange(boolean enabled) {
return this;
}
+ /**
+ * Whether to enable bookie address changes tracking.
+ *
+ * @return flag to enable/disable bookie address changes tracking
+ */
+ public boolean getEnableBookieAddressTracking() {
+ return getBoolean(FOLLOW_BOOKIE_ADDRESS_TRACKING, true);
+ }
+
+ /**
+ * Enable/Disable bookie address changes tracking.
+ *
+ * @param enabled
+ * flag to enable/disable bookie address changes tracking
+ * @return client configuration.
+ */
+ public ClientConfiguration setEnableBookieAddressTracking(boolean enabled) {
Review comment:
nit: Value set can be true/false enabling/disabling, not sure if its clear to name variable as `enabled`
----------------------------------------------------------------
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] Ghatage commented on a change in pull request #2435: BP-41 Bookie Address changes tracking
Posted by GitBox <gi...@apache.org>.
Ghatage commented on a change in pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435#discussion_r504782501
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/discover/ZKRegistrationClient.java
##########
@@ -447,4 +461,46 @@ public synchronized void unwatchReadOnlyBookies(RegistrationListener listener) {
return newBookieAddrs;
}
+ private static BookieId stripBookieIdFromPath(String path) {
+ final int slash = path.lastIndexOf('/');
+ if (slash >= 0) {
+ try {
+ return BookieId.parse(path.substring(slash + 1));
+ } catch (IllegalArgumentException e) {
+ log.warn("Cannot decode bookieId from {}", path, e);
+ }
+ }
+ return null;
+ }
+
+ private class BookieServiceInfoCacheInvalidationWatcher implements Watcher {
+
Review comment:
How frequently can a bookieID change? What factors drive its change?
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java
##########
@@ -1764,6 +1767,27 @@ public ClientConfiguration setDelayEnsembleChange(boolean enabled) {
return this;
}
+ /**
+ * Whether to enable bookie address changes tracking.
+ *
+ * @return flag to enable/disable bookie address changes tracking
+ */
+ public boolean getEnableBookieAddressTracking() {
+ return getBoolean(FOLLOW_BOOKIE_ADDRESS_TRACKING, true);
+ }
+
+ /**
+ * Enable/Disable bookie address changes tracking.
+ *
+ * @param enabled
+ * flag to enable/disable bookie address changes tracking
+ * @return client configuration.
+ */
+ public ClientConfiguration setEnableBookieAddressTracking(boolean enabled) {
Review comment:
nit: We can use this to enable/disable, so not sure if naming variable as `enabled` is clear. It could just be `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] eolivelli commented on a change in pull request #2435: BP-41 Bookie Address changes tracking
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435#discussion_r504794750
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/discover/ZKRegistrationClient.java
##########
@@ -447,4 +461,46 @@ public synchronized void unwatchReadOnlyBookies(RegistrationListener listener) {
return newBookieAddrs;
}
+ private static BookieId stripBookieIdFromPath(String path) {
+ final int slash = path.lastIndexOf('/');
+ if (slash >= 0) {
+ try {
+ return BookieId.parse(path.substring(slash + 1));
+ } catch (IllegalArgumentException e) {
+ log.warn("Cannot decode bookieId from {}", path, e);
+ }
+ }
+ return null;
+ }
+
+ private class BookieServiceInfoCacheInvalidationWatcher implements Watcher {
+
Review comment:
I see these known cases:
- a bookie with a dynamic network address
- changing configuration (change DNS name....)
- you change configuration and add new endpoints (this will happen in the future, when for instance @sijie will contribute the pure TLS endpoint)
So basically I think they won't change very ofter
----------------------------------------------------------------
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] Ghatage edited a comment on pull request #2435: BP-41 Bookie Address changes tracking
Posted by GitBox <gi...@apache.org>.
Ghatage edited a comment on pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435#issuecomment-708508804
> If you configure bookieId in your bookie then only 4.12 clients will be able to use it.
So can there be a case where we upgrade Bookies to 4.12 but disable this option and use the older `hostname:port` pairing
But then we won't be able to use 4.12 clients?
Sorry if I am misunderstanding this!
----------------------------------------------------------------
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 edited a comment on pull request #2435: BP-41 Follow Bookie Address changes tracking
Posted by GitBox <gi...@apache.org>.
eolivelli edited a comment on pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435#issuecomment-706520715
also @nicoloboschi @Ghatage PTAL
----------------------------------------------------------------
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 #2435: BP-41 Bookie Address changes tracking
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435#issuecomment-708444106
gently nudging at @sijie @rdhabalia @jiazhai
----------------------------------------------------------------
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 #2435: BP-41 Bookie Address changes tracking
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435#issuecomment-710098010
@jiazhai @rdhabalia Please help me merging this patch
----------------------------------------------------------------
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] Ghatage commented on pull request #2435: BP-41 Bookie Address changes tracking
Posted by GitBox <gi...@apache.org>.
Ghatage commented on pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435#issuecomment-708508804
> If you configure bookieId in your bookie then only 4.12 clients will be able to use it.
So can there be a case where we upgrade Bookies to 4.12 but disable this option and use the older `hostname:port` pairing
But then we won't be able to use 4.12 clients?
----------------------------------------------------------------
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 #2435: BP-41 Bookie Address changes tracking
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435#discussion_r504796972
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java
##########
@@ -1764,6 +1767,27 @@ public ClientConfiguration setDelayEnsembleChange(boolean enabled) {
return this;
}
+ /**
+ * Whether to enable bookie address changes tracking.
+ *
+ * @return flag to enable/disable bookie address changes tracking
+ */
+ public boolean getEnableBookieAddressTracking() {
+ return getBoolean(FOLLOW_BOOKIE_ADDRESS_TRACKING, true);
+ }
+
+ /**
+ * Enable/Disable bookie address changes tracking.
+ *
+ * @param enabled
+ * flag to enable/disable bookie address changes tracking
+ * @return client configuration.
+ */
+ public ClientConfiguration setEnableBookieAddressTracking(boolean enabled) {
Review comment:
changed
----------------------------------------------------------------
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 #2435: BP-41 Bookie Address changes tracking
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2435:
URL: https://github.com/apache/bookkeeper/pull/2435#issuecomment-708503305
> Can you please explain the upgrade scenario for this?
> As I understand, after upgrading the bits we can still use the older bookie id but if we want, we can start the bookies with BP-41 enabled, and then we have to restart the clients with it enabled too? Is that right?
If you don't configure the bookieId the bookie will work as it worked in 4.11, so no problem for old clients.
If you configure bookieId in your bookie then only 4.12 clients will be able to use it.
If you want to leverage Bookie Address Change Tracking you have to:
- use bookidId
- use 4.12
- enable it in ClientConfiguration (it is enabled by default, in order to be friendly with bookies with dynamic addresses by default)
We cannot set a bookieId by default in order to be backward compatible
----------------------------------------------------------------
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