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 2021/12/16 12:15:16 UTC
[GitHub] [bookkeeper] lordcheng10 opened a new pull request #2947: suport dynamic enable/disable health check
lordcheng10 opened a new pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947
### Motivation
When upgrading bookkeeper, you need to restart the bookie nodes in turn. When restarting a bookie, the pulsar will fail to read and write to the bookie, so as to isolate the bookie node. The default isolation is 30 minutes. With more and more restarted bookie nodes, the traffic of the whole cluster will be transferred to the remaining few bookie nodes, At this time, the bookie node may be abnormal due to excessive pressure, such as pulsar read-write timeout.
### Changes
Therefore, I provide a configuration to dynamically turn off the health check. When upgrading bookkeeper, turn off the isolation function and turn it on after the upgrade, so as to avoid the impact of upgrading bookkeeper on the stability of pulsar.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1014276760
@lordcheng10
committed to master branch
would you mind to follow up with adding docs for this new feature ?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r771083991
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
##########
@@ -525,6 +527,15 @@ public void setZkLedgersRootPath(String zkLedgersPath) {
setProperty(ZK_LEDGERS_ROOT_PATH, zkLedgersPath);
}
+ /**
+ * @return zk enable health path
+ * */
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(this);
+ String ledgerParentPath = zkLedgersRootPath.substring(0, zkLedgersRootPath.lastIndexOf("/ledgers"));
+ return String.format("%s/%s", ledgerParentPath, "enableHealthCheck");
Review comment:
add a new method :
public static String resolveMetadataServiceRootPath(AbstractConfiguration<?> conf) {
String metadataServiceUriStr = conf.getMetadataServiceUriUnchecked();
URI metadataServiceUri = URI.create(metadataServiceUriStr);
return metadataServiceUri.getPath();
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773710130
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -614,7 +614,19 @@ public void safeRun() {
}
}
- void checkForFaultyBookies() {
+ void checkForFaultyBookies() {
+ boolean isEnable = true;
+ try {
+ isEnable = metadataDriver.isEnableHealthCheck().get();
+ } catch (Exception e) {
+ LOG.error("call method isEnableHealthCheck failed!", e);
Review comment:
like this?
LOG.error("metadataDriver.isEnableHealthCheck() failed，isEnable is treated as true !", e);
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774907915
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
it is better to use the async API of ZooKeeper, this is why I told you about using CompletableFuture
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ log.debug("health check already disable!");
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(disableHealthCheckPath, -1);
+ deleteResult.complete(null);
+ } catch (KeeperException.NoNodeException noNodeException) {
+ log.debug("health check already enabled!");
+ deleteResult.complete(null);
+ } catch (Exception e) {
+ deleteResult.completeExceptionally(e);
+ }
+ return deleteResult;
+ }
+
+ public CompletableFuture<Boolean> isHealthCheckEnabled() {
+ CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
+ try {
+ boolean isEnable = (null == zk.exists(disableHealthCheckPath, false));
Review comment:
please use the async API
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ log.debug("health check already disable!");
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(disableHealthCheckPath, -1);
Review comment:
please use the async API
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774942934
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
fixed method :
` public CompletableFuture<Void> disableHealthCheck() {
CompletableFuture<Void> createResult = new CompletableFuture<>();
zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT, new AsyncCallback.StringCallback() {
@Override
public void processResult(int rc, String path, Object ctx, String name) {
if (KeeperException.Code.OK.intValue() == rc) {
createResult.complete(null);
} else if (KeeperException.Code.NODEEXISTS.intValue() == rc) {
log.debug("health check already disable!");
createResult.complete(null);
} else {
createResult.completeExceptionally(KeeperException.create(KeeperException.Code.get(rc), path));
}
}
}, null);
return createResult;
}`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1000652628
PTAL, thanks ! @eolivelli @nicoloboschi
some changes again :
1. rename enableHealthCheckPath to disableHealthCheckPath;
2. call getFaultyBookies method first In the checkForFaultyBookies(), this method call can reset errorCounter to 0:
https://github.com/apache/bookkeeper/blob/4dae979c5b35c434105f0c9810c87dd91237bb7e/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClientImpl.java#L160-L163
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774911451
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
you are right! I will fix!
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ log.debug("health check already disable!");
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(disableHealthCheckPath, -1);
Review comment:
I will fix
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ log.debug("health check already disable!");
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(disableHealthCheckPath, -1);
+ deleteResult.complete(null);
+ } catch (KeeperException.NoNodeException noNodeException) {
+ log.debug("health check already enabled!");
+ deleteResult.complete(null);
+ } catch (Exception e) {
+ deleteResult.completeExceptionally(e);
+ }
+ return deleteResult;
+ }
+
+ public CompletableFuture<Boolean> isHealthCheckEnabled() {
+ CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
+ try {
+ boolean isEnable = (null == zk.exists(disableHealthCheckPath, false));
Review comment:
I will fix
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773808104
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
Review comment:
like this? @nicoloboschi
public CompletableFuture<Void> disableHealthCheck() {
CompletableFuture<Void> createResult = new CompletableFuture<>();
try {
zk.create(getEnableHealthPath(), BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
createResult.complete(null);
} catch (KeeperException.NodeExistsException nodeExistsException) {
createResult.complete(null);
} catch (Exception e) {
createResult.completeExceptionally(e);
}
return createResult;
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773713798
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
Review comment:
OK, I will use BookKeeperConstants.EMPTY_BYTE_ARRAY replace new byte[0].
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773714370
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
Review comment:
I will fixed
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773850192
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +269,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(enableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ log.debug("enableHealthCheckPath {} is existed!", enableHealthCheckPath);
Review comment:
you are right!
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +269,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(enableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ log.debug("enableHealthCheckPath {} is existed!", enableHealthCheckPath);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(enableHealthCheckPath, -1);
+ deleteResult.complete(null);
+ } catch (KeeperException.NoNodeException noNodeException) {
+ log.debug("enableHealthCheckPath {} is not existed!", enableHealthCheckPath);
Review comment:
you are right!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773793984
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
+ deleteResult.completeExceptionally(e);
+ }
+ return deleteResult;
+ }
+
+ public CompletableFuture<Boolean> isEnableHealthCheck() {
+ CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
+ try {
+ boolean isEnable = (null == zk.exists(getEnableHealthPath(), false));
+ enableResult.complete(isEnable);
+ } catch (Exception e) {
+ enableResult.completeExceptionally(e);
+ }
+ return enableResult;
+ }
+
+
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(conf);
Review comment:
OK ,I will fixed
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1003568251
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1009591654
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1013259904
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r783771488
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,29 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() throws Exception {
Review comment:
like this?
/**
* Disable health check.
*/
default CompletableFuture<Void> disableHealthCheck() {
CompletableFuture<Void> result = new CompletableFuture<>();
result.completeExceptionally(new Exception("Not support disableHealthCheck"));
return result;
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1012880786
@pkumar-singh @dlg99 PTAL,thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r777295246
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,29 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
Review comment:
In class org apache.bookkeeper.tools.cli.commands.autorecovery，also used MetadataBookieDriver。
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774736078
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(enableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
Should not this be ephemeral. So that when bookie goes away and comes back again, should come back in health check enabled mode?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774837857
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(enableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
PTAL, thanks ! @pkumar-singh
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 removed a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 removed a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1000652628
PTAL, thanks ! @eolivelli @nicoloboschi
some changes again :
1. rename enableHealthCheckPath to disableHealthCheckPath;
2. call getFaultyBookies method first In the checkForFaultyBookies(), this method call can reset errorCounter to 0:
https://github.com/apache/bookkeeper/blob/4dae979c5b35c434105f0c9810c87dd91237bb7e/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClientImpl.java#L160-L163
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1000420792
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1000681420
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774951002
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ log.debug("health check already disable!");
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(disableHealthCheckPath, -1);
+ deleteResult.complete(null);
+ } catch (KeeperException.NoNodeException noNodeException) {
+ log.debug("health check already enabled!");
+ deleteResult.complete(null);
+ } catch (Exception e) {
+ deleteResult.completeExceptionally(e);
+ }
+ return deleteResult;
+ }
+
+ public CompletableFuture<Boolean> isHealthCheckEnabled() {
+ CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
+ try {
+ boolean isEnable = (null == zk.exists(disableHealthCheckPath, false));
Review comment:
public CompletableFuture<Boolean> isHealthCheckEnabled() {
CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
zk.exists(disableHealthCheckPath, false, new AsyncCallback.StatCallback() {
@Override
public void processResult(int rc, String path, Object ctx, Stat stat) {
if (KeeperException.Code.OK.intValue() == rc) {
enableResult.complete(false);
} else {
enableResult.complete(true);
}
}
}, null);
return enableResult;
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-996362181
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r771914920
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
##########
@@ -525,6 +527,15 @@ public void setZkLedgersRootPath(String zkLedgersPath) {
setProperty(ZK_LEDGERS_ROOT_PATH, zkLedgersPath);
}
+ /**
+ * @return zk enable health path
+ * */
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(this);
Review comment:
OK, I will move the getEnableHealthPath method implementation to the ZKMetadataDriverBase class.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773710130
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -614,7 +614,19 @@ public void safeRun() {
}
}
- void checkForFaultyBookies() {
+ void checkForFaultyBookies() {
+ boolean isEnable = true;
+ try {
+ isEnable = metadataDriver.isEnableHealthCheck().get();
+ } catch (Exception e) {
+ LOG.error("call method isEnableHealthCheck failed!", e);
Review comment:
like this? @nicoloboschi
LOG.error("metadataDriver.isEnableHealthCheck() failed，isEnable is treated as true !", e);
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773801951
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
+ deleteResult.completeExceptionally(e);
+ }
+ return deleteResult;
+ }
+
+ public CompletableFuture<Boolean> isEnableHealthCheck() {
+ CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
+ try {
+ boolean isEnable = (null == zk.exists(getEnableHealthPath(), false));
+ enableResult.complete(isEnable);
+ } catch (Exception e) {
+ enableResult.completeExceptionally(e);
+ }
+ return enableResult;
+ }
+
+
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(conf);
Review comment:
The same directory seems to be defined, can I use ledgerRootPath directly? @nicoloboschi
https://github.com/apache/bookkeeper/blob/370d785ff674841943fa8cddee1964408e826f5b/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java#L143-L145
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r771083991
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
##########
@@ -525,6 +527,15 @@ public void setZkLedgersRootPath(String zkLedgersPath) {
setProperty(ZK_LEDGERS_ROOT_PATH, zkLedgersPath);
}
+ /**
+ * @return zk enable health path
+ * */
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(this);
+ String ledgerParentPath = zkLedgersRootPath.substring(0, zkLedgersRootPath.lastIndexOf("/ledgers"));
+ return String.format("%s/%s", ledgerParentPath, "enableHealthCheck");
Review comment:
add a new method :
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
##########
@@ -525,6 +527,15 @@ public void setZkLedgersRootPath(String zkLedgersPath) {
setProperty(ZK_LEDGERS_ROOT_PATH, zkLedgersPath);
}
+ /**
+ * @return zk enable health path
+ * */
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(this);
+ String ledgerParentPath = zkLedgersRootPath.substring(0, zkLedgersRootPath.lastIndexOf("/ledgers"));
+ return String.format("%s/%s", ledgerParentPath, "enableHealthCheck");
Review comment:
add a new method :
public static String resolveMetadataServiceRootPath(AbstractConfiguration<?> conf) {
String metadataServiceUriStr = conf.getMetadataServiceUriUnchecked();
URI metadataServiceUri = URI.create(metadataServiceUriStr);
return metadataServiceUri.getPath();
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1001347423
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1004478429
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1004472313
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1014317546
> @lordcheng10
> committed to master branch
>
> would you mind to follow up with adding docs for this new feature ?
OK I will add docs
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r785283143
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/health/SwitchOfHealthCheckCommand.java
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.tools.cli.commands.health;
+
+import com.beust.jcommander.Parameter;
+import com.google.common.util.concurrent.UncheckedExecutionException;
+import java.util.concurrent.ExecutionException;
+import lombok.Setter;
+import lombok.experimental.Accessors;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.meta.MetadataDrivers;
+import org.apache.bookkeeper.meta.exceptions.MetadataException;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.tools.framework.CliFlags;
+import org.apache.bookkeeper.tools.framework.CliSpec;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+
+/**
+ * Command to enable or disable health check in the cluster.
+ */
+public class SwitchOfHealthCheckCommand extends BookieCommand<SwitchOfHealthCheckCommand.HealthCheckFlags> {
+
+ static final Logger LOG = LoggerFactory.getLogger(SwitchOfHealthCheckCommand.class);
+
+ private static final String NAME = "switch";
+ private static final String DESC = "Enable or disable health check in the cluster. Default is enable.";
+
+ public SwitchOfHealthCheckCommand() {
+ this(new HealthCheckFlags());
+ }
+
+ private SwitchOfHealthCheckCommand(HealthCheckFlags flags) {
+ super(CliSpec.<HealthCheckFlags>newBuilder()
+ .withName(NAME).withDescription(DESC)
+ .withFlags(flags).build());
+ }
+
+ /**
+ * Flags for health check command.
+ */
+ @Accessors(fluent = true)
+ @Setter
+ public static class HealthCheckFlags extends CliFlags {
+
+ @Parameter(names = { "-e", "--enable" }, description = "Enable or disable health check.")
+ private boolean enable;
+
+ @Parameter(names = {"-s", "--status"}, description = "Check the health check status.")
+ private boolean status;
+
+ }
+
+ @Override
+ public boolean apply(ServerConfiguration conf, HealthCheckFlags cmdFlags) {
+ try {
+ return handler(conf, cmdFlags);
+ } catch (MetadataException | ExecutionException e) {
+ throw new UncheckedExecutionException(e.getMessage(), e);
+ }
+ }
+
+ private boolean handler(ServerConfiguration conf, HealthCheckFlags flags)
+ throws MetadataException, ExecutionException {
+
+ MetadataDrivers.runFunctionWithMetadataBookieDriver(conf, driver -> {
+ try {
+ boolean isEnable = driver.isHealthCheckEnabled().get();
+
+ if (flags.status) {
+ LOG.info("EnableHealthCheck is " + (isEnable ? "enabled." : "disabled."));
+ return null;
+ }
+
+ if (flags.enable) {
+ if (isEnable) {
+ LOG.warn("HealthCheck already enable. Doing nothing");
Review comment:
OK I will fix
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/health/SwitchOfHealthCheckCommand.java
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.tools.cli.commands.health;
+
+import com.beust.jcommander.Parameter;
+import com.google.common.util.concurrent.UncheckedExecutionException;
+import java.util.concurrent.ExecutionException;
+import lombok.Setter;
+import lombok.experimental.Accessors;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.meta.MetadataDrivers;
+import org.apache.bookkeeper.meta.exceptions.MetadataException;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.tools.framework.CliFlags;
+import org.apache.bookkeeper.tools.framework.CliSpec;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+
+/**
+ * Command to enable or disable health check in the cluster.
+ */
+public class SwitchOfHealthCheckCommand extends BookieCommand<SwitchOfHealthCheckCommand.HealthCheckFlags> {
+
+ static final Logger LOG = LoggerFactory.getLogger(SwitchOfHealthCheckCommand.class);
+
+ private static final String NAME = "switch";
+ private static final String DESC = "Enable or disable health check in the cluster. Default is enable.";
+
+ public SwitchOfHealthCheckCommand() {
+ this(new HealthCheckFlags());
+ }
+
+ private SwitchOfHealthCheckCommand(HealthCheckFlags flags) {
+ super(CliSpec.<HealthCheckFlags>newBuilder()
+ .withName(NAME).withDescription(DESC)
+ .withFlags(flags).build());
+ }
+
+ /**
+ * Flags for health check command.
+ */
+ @Accessors(fluent = true)
+ @Setter
+ public static class HealthCheckFlags extends CliFlags {
+
+ @Parameter(names = { "-e", "--enable" }, description = "Enable or disable health check.")
+ private boolean enable;
+
+ @Parameter(names = {"-s", "--status"}, description = "Check the health check status.")
+ private boolean status;
+
+ }
+
+ @Override
+ public boolean apply(ServerConfiguration conf, HealthCheckFlags cmdFlags) {
+ try {
+ return handler(conf, cmdFlags);
+ } catch (MetadataException | ExecutionException e) {
+ throw new UncheckedExecutionException(e.getMessage(), e);
+ }
+ }
+
+ private boolean handler(ServerConfiguration conf, HealthCheckFlags flags)
+ throws MetadataException, ExecutionException {
+
+ MetadataDrivers.runFunctionWithMetadataBookieDriver(conf, driver -> {
+ try {
+ boolean isEnable = driver.isHealthCheckEnabled().get();
+
+ if (flags.status) {
+ LOG.info("EnableHealthCheck is " + (isEnable ? "enabled." : "disabled."));
+ return null;
+ }
+
+ if (flags.enable) {
+ if (isEnable) {
+ LOG.warn("HealthCheck already enable. Doing nothing");
+ } else {
+ LOG.info("Enable HealthCheck");
+ driver.enableHealthCheck().get();
+ }
+ } else {
+ if (!isEnable) {
+ LOG.warn("HealthCheck already disable. Doing nothing");
Review comment:
OK I will fix
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 removed a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 removed a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1010398012
@michaeljmarshall PTAL，thanks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r777226086
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataClientDriver.java
##########
@@ -103,4 +105,28 @@ LedgerManagerFactory getLedgerManagerFactory()
* listener listening on metadata client session states.
*/
void setSessionStateListener(SessionStateListener sessionStateListener);
+
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
+ return FutureUtils.Void();
Review comment:
like this?
/**
* Disable health check.
*/
default CompletableFuture<Void> disableHealthCheck() {
new Exception("Not support disableHealthCheck");
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1004056692
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1003928389
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1004244271
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1004719961
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-995762267
@nicoloboschi @dlg99 @eolivelli @Shoothzj
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-996035493
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-996003630
According to your suggestion, I modified it accordingly. Please review again，thanks! @eolivelli
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r771946571
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +260,21 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ @SneakyThrows
Review comment:
OK, I will fixed.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774830759
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(enableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
Here, we restart with bookie as a condition for restarting the Health Check, which, in my opinion, is not reasonable.
Consider an exception case: a bookie reconnects frequently with ZooKeeper
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1000652628
I made two changes:
1. rename enableHealthCheckPath to disableHealthCheckPath;
2.Need to call getFaultyBookies method first, this method call can reset errorCounter to 0:
https://github.com/apache/bookkeeper/blob/4dae979c5b35c434105f0c9810c87dd91237bb7e/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClientImpl.java#L160-L163
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774831933
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(enableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
The purpose of this PR is to turn off the Health Check during the BookKeeper roller-restart upgrade and to enable it again after the upgrade is complete.
Bookie coming back is not a sign that the upgrade is complete
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774950591
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
public CompletableFuture<Void> disableHealthCheck() {
CompletableFuture<Void> createResult = new CompletableFuture<>();
zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls,
CreateMode.PERSISTENT, new AsyncCallback.StringCallback() {
public void processResult(int rc, String path, Object ctx, String name) {
if (KeeperException.Code.OK.intValue() == rc) {
createResult.complete(null);
} else if (KeeperException.Code.NODEEXISTS.intValue() == rc) {
log.debug("health check already disable!");
createResult.complete(null);
} else {
createResult.completeExceptionally(KeeperException.create(KeeperException.Code.get(rc), path));
}
}
}, null);
return createResult;
}
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
like this ?
public CompletableFuture<Void> disableHealthCheck() {
CompletableFuture<Void> createResult = new CompletableFuture<>();
zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls,
CreateMode.PERSISTENT, new AsyncCallback.StringCallback() {
public void processResult(int rc, String path, Object ctx, String name) {
if (KeeperException.Code.OK.intValue() == rc) {
createResult.complete(null);
} else if (KeeperException.Code.NODEEXISTS.intValue() == rc) {
log.debug("health check already disable!");
createResult.complete(null);
} else {
createResult.completeExceptionally(KeeperException.create(KeeperException.Code.get(rc), path));
}
}
}, null);
return createResult;
}
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
public CompletableFuture<Void> disableHealthCheck() {
CompletableFuture<Void> createResult = new CompletableFuture<>();
zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls,
CreateMode.PERSISTENT, new AsyncCallback.StringCallback() {
public void processResult(int rc, String path, Object ctx, String name) {
if (KeeperException.Code.OK.intValue() == rc) {
createResult.complete(null);
} else if (KeeperException.Code.NODEEXISTS.intValue() == rc) {
log.debug("health check already disable!");
createResult.complete(null);
} else {
createResult.completeExceptionally(KeeperException.create(KeeperException.Code.get(rc), path));
}
}
}, null);
return createResult;
}
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
public CompletableFuture<Void> disableHealthCheck() {
CompletableFuture<Void> createResult = new CompletableFuture<>();
zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls,
CreateMode.PERSISTENT, new AsyncCallback.StringCallback() {
@Override
public void processResult(int rc, String path, Object ctx, String name) {
if (KeeperException.Code.OK.intValue() == rc) {
createResult.complete(null);
} else if (KeeperException.Code.NODEEXISTS.intValue() == rc) {
log.debug("health check already disable!");
createResult.complete(null);
} else {
createResult.completeExceptionally(KeeperException.create(KeeperException.Code.get(rc), path));
}
}
}, null);
return createResult;
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r777226086
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataClientDriver.java
##########
@@ -103,4 +105,28 @@ LedgerManagerFactory getLedgerManagerFactory()
* listener listening on metadata client session states.
*/
void setSessionStateListener(SessionStateListener sessionStateListener);
+
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
+ return FutureUtils.Void();
Review comment:
like this:
/**
* Disable health check.
*/
default CompletableFuture<Void> disableHealthCheck() {
new Exception("Not support disableHealthCheck");
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r777224201
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,29 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
Review comment:
This method metadatabookiedriver disablehealthcheck is used in the class switchofhealthcheckcommand。
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1003884438
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1003914563
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1007961260
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1011783156
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r783946785
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -616,6 +617,23 @@ public void safeRun() {
void checkForFaultyBookies() {
List<BookieId> faultyBookies = bookieClient.getFaultyBookies();
+ if (faultyBookies.isEmpty()) {
+ return;
+ }
+
+ boolean isEnable = false;
+ try {
+ isEnable = metadataDriver.isHealthCheckEnabled().get();
+ } catch (InterruptedException e) {
+ LOG.error("isHealthCheckEnabled throw InterruptedException", e);
+ } catch (ExecutionException e) {
+ LOG.error("isHealthCheckEnabled throw ExecutionException", e);
Review comment:
OK I will fix
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,31 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> result = new CompletableFuture<>();
+ result.completeExceptionally(new Exception("Not support disableHealthCheck"));
Review comment:
OK I will fix
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r783945832
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,31 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> result = new CompletableFuture<>();
+ result.completeExceptionally(new Exception("Not support disableHealthCheck"));
Review comment:
result.completeExceptionally(new Exception("disableHealthCheck is not supported by this metadata driver"));
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1013013643
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1012835145
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1014018789
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli merged pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r783737778
##########
File path: bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/SafeRunnable.java
##########
@@ -39,7 +40,7 @@ default void run() {
}
}
- void safeRun();
+ void safeRun() throws ExecutionException, InterruptedException;
Review comment:
please don't change safeRun this way.
it is a core utility, there is no need to change this here:in any case you can catch your exception and handle them properly. This code is generic and the caller could not react correctly to exceptions, so it is useless to say that the method may throw checked exceptions
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -606,16 +607,26 @@ void scheduleBookieHealthCheckIfEnabled(ClientConfiguration conf) {
scheduler.scheduleAtFixedRate(new SafeRunnable() {
@Override
- public void safeRun() {
+ public void safeRun() throws ExecutionException, InterruptedException {
checkForFaultyBookies();
}
}, conf.getBookieHealthCheckIntervalSeconds(), conf.getBookieHealthCheckIntervalSeconds(),
TimeUnit.SECONDS);
}
}
- void checkForFaultyBookies() {
+ void checkForFaultyBookies() throws ExecutionException, InterruptedException {
List<BookieId> faultyBookies = bookieClient.getFaultyBookies();
+ if (faultyBookies.isEmpty()) {
+ return;
+ }
+
+ boolean isEnable = metadataDriver.isHealthCheckEnabled().get();
Review comment:
you can catch "ExecutionException, InterruptedException" here and do not add the throws clause to checkForFaultyBookies()
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,29 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() throws Exception {
Review comment:
methods that return CompletableFuture should not throw Exceptions.
you have to return a failed CompletableFuture
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r783771488
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,29 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() throws Exception {
Review comment:
like 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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1003878597
> I left my last comments then I believe that we are good to go
Thank you so much for your suggestions. Great idea！@eolivelli
According to your suggestions, I made the following changes:
1. Don't catch raw Exception when call method of metadataDriver.isHealthCheckEnabled().get();
2.Default action in method of disableHealthCheck is throw exception:
default CompletableFuture<Void> disableHealthCheck() throws Exception {
throw new Exception("Not support disableHealthCheck");
}
3.the methods disableHealthCheck and enableHealthCheck in the class org.apache.bookkeeper.meta.MetadataClientDriver are not used and has been removed. org.apache.bookkeeper.meta.MetadataBookieDriver#disableHealthCheck() is called in the class org.apache.bookkeeper .tools.cli.commands.health.SwitchOfHealthCheckCommand, so it cannot be removed。
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1003878597
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1003878597
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-995767516
With this command, you can turn off the health check of the client, so as to avoid isolating the bookie when upgrading the bookie:
# enable health check
sh bkctl healthCheck switch -e
#disable helth check
sh bkctl healthCheck switch
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-995767516
With this command, you can turn off the health check of the client, so as to avoid isolating the bookie when upgrading the bookie:
sh bkctl healthCheck switch -d
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r777226086
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataClientDriver.java
##########
@@ -103,4 +105,28 @@ LedgerManagerFactory getLedgerManagerFactory()
* listener listening on metadata client session states.
*/
void setSessionStateListener(SessionStateListener sessionStateListener);
+
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
+ return FutureUtils.Void();
Review comment:
like this?
default CompletableFuture<Void> disableHealthCheck() throws Exception {
throw new Exception("Not support disableHealthCheck");
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1005054324
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r771947958
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +68,27 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default boolean isEnableHealthCheck() {
+ return true;
+ }
+
+ /**
+ * Disable health check.
+ */
+ default void disableHealthCheck(String enableHealthPath) {
Review comment:
OK, I will fixed
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-997392339
> Please address Enrico's comments, otherwise LGTM.
Following Enrico’s suggestion, I modified it accordingly
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-997392512
> Following Enrico’s suggestion, I modified it accordingly
Following Enrico’s suggestion, I modified it accordingly。
PTAL,thanks! @eolivelli
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r785283141
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/health/SwitchOfHealthCheckCommand.java
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.tools.cli.commands.health;
+
+import com.beust.jcommander.Parameter;
+import com.google.common.util.concurrent.UncheckedExecutionException;
+import java.util.concurrent.ExecutionException;
+import lombok.Setter;
+import lombok.experimental.Accessors;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.meta.MetadataDrivers;
+import org.apache.bookkeeper.meta.exceptions.MetadataException;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.tools.framework.CliFlags;
+import org.apache.bookkeeper.tools.framework.CliSpec;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+
+/**
+ * Command to enable or disable health check in the cluster.
+ */
+public class SwitchOfHealthCheckCommand extends BookieCommand<SwitchOfHealthCheckCommand.HealthCheckFlags> {
+
+ static final Logger LOG = LoggerFactory.getLogger(SwitchOfHealthCheckCommand.class);
+
+ private static final String NAME = "switch";
+ private static final String DESC = "Enable or disable health check in the cluster. Default is enable.";
Review comment:
OK I will fix
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1013638649
Thank you for your suggestion. According to your suggestion I have made the corresponding changes，please review again！@dlg99
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773801951
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
+ deleteResult.completeExceptionally(e);
+ }
+ return deleteResult;
+ }
+
+ public CompletableFuture<Boolean> isEnableHealthCheck() {
+ CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
+ try {
+ boolean isEnable = (null == zk.exists(getEnableHealthPath(), false));
+ enableResult.complete(isEnable);
+ } catch (Exception e) {
+ enableResult.completeExceptionally(e);
+ }
+ return enableResult;
+ }
+
+
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(conf);
Review comment:
The same path seems to be defined, can I use ledgerRootPath directly? @nicoloboschi
https://github.com/apache/bookkeeper/blob/370d785ff674841943fa8cddee1964408e826f5b/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java#L143-L145
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
+ deleteResult.completeExceptionally(e);
+ }
+ return deleteResult;
+ }
+
+ public CompletableFuture<Boolean> isEnableHealthCheck() {
+ CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
+ try {
+ boolean isEnable = (null == zk.exists(getEnableHealthPath(), false));
+ enableResult.complete(isEnable);
+ } catch (Exception e) {
+ enableResult.completeExceptionally(e);
+ }
+ return enableResult;
+ }
+
+
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(conf);
+ String ledgerParentPath = zkLedgersRootPath.substring(0, zkLedgersRootPath.lastIndexOf("/ledgers"));
+ return String.format("%s/%s", ledgerParentPath, "enableHealthCheck");
Review comment:
OK,I will fixed
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-996039678
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1004472213
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1006218554
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r783945877
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -616,6 +617,23 @@ public void safeRun() {
void checkForFaultyBookies() {
List<BookieId> faultyBookies = bookieClient.getFaultyBookies();
+ if (faultyBookies.isEmpty()) {
+ return;
+ }
+
+ boolean isEnable = false;
+ try {
+ isEnable = metadataDriver.isHealthCheckEnabled().get();
+ } catch (InterruptedException e) {
+ LOG.error("isHealthCheckEnabled throw InterruptedException", e);
Review comment:
OK
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1010383134
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1011994709
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 removed a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 removed a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1014064847
@zymap This PR has been open for a long time. If there is no problem, can you merge it for me? thank you!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773713044
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,36 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isEnableHealthCheck() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
+ return FutureUtils.Void();
+ }
+
+ /**
+ * Enable health check.
+ */
+ default CompletableFuture<Void> enableHealthCheck() {
+ return FutureUtils.Void();
+ }
+
+ /**
+ * @return disable health check path
+ */
+ default String getEnableHealthPath() {
Review comment:
You are right! I will remove getEnableHealthPath in MetadataBookieDriver
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773814238
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
Review comment:
yes, we can add a debug log in that case
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773814117
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
Review comment:
yes, we can add a debug log in that case
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1002366550
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1000652628
PTAL, thanks ! @eolivelli @nicoloboschi
some changes again :
1. rename enableHealthCheckPath to disableHealthCheckPath;
2. Need to call getFaultyBookies method first In the checkForFaultyBookies(), this method call can reset errorCounter to 0:
https://github.com/apache/bookkeeper/blob/4dae979c5b35c434105f0c9810c87dd91237bb7e/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClientImpl.java#L160-L163
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1000652628
I made two changes:
1. rename enableHealthCheckPath to disableHealthCheckPath;
2. Need to call getFaultyBookies method first In the checkForFaultyBookies(), this method call can reset errorCounter to 0:
https://github.com/apache/bookkeeper/blob/4dae979c5b35c434105f0c9810c87dd91237bb7e/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClientImpl.java#L160-L163
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773804615
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -614,7 +614,19 @@ public void safeRun() {
}
}
- void checkForFaultyBookies() {
+ void checkForFaultyBookies() {
+ boolean isEnable = true;
+ try {
+ isEnable = metadataDriver.isEnableHealthCheck().get();
+ } catch (Exception e) {
+ LOG.error("call method isEnableHealthCheck failed!", e);
Review comment:
OK,I will fixed!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1000681505
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1003061057
Please review again, thank you ! @eolivelli
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r777295594
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,29 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
Review comment:
In fact, the methods disableHealthCheck and enableHealthCheck in the class org.apache.bookkeeper.meta.MetadataClientDriver are not used and can be removed
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r770513971
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -614,7 +615,19 @@ public void safeRun() {
}
}
+ boolean enableHealthCheck() {
+ if (metadataDriver instanceof ZKMetadataClientDriver) {
Review comment:
why this "instanceof" ?
please add the method in the MetadataClientDriver class
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +261,30 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public boolean isEnableHealthCheck() {
+ try {
+ return null == zk.exists(conf.getEnableHealthPath(), false);
+ } catch (Exception e) {
+ return true;
+ }
+ }
+
+ public void disableHealthCheck(String enableHealthPath) throws Exception{
+ zk.create(enableHealthPath, new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
Review comment:
there should be a configuration flag to define which ACL we should use, please check
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -614,7 +615,19 @@ public void safeRun() {
}
}
+ boolean enableHealthCheck() {
+ if (metadataDriver instanceof ZKMetadataClientDriver) {
+ return ((ZKMetadataClientDriver) metadataDriver).enableHealthCheck();
+ }
+ return true;
+ }
+
void checkForFaultyBookies() {
+ if (!enableHealthCheck()) {
+ LOG.info("enableHealthCheck is false!");
Review comment:
What about: "Health checks is currently disabled"
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +261,30 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public boolean isEnableHealthCheck() {
+ try {
+ return null == zk.exists(conf.getEnableHealthPath(), false);
+ } catch (Exception e) {
+ return true;
+ }
+ }
+
+ public void disableHealthCheck(String enableHealthPath) throws Exception{
+ zk.create(enableHealthPath, new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+
+ public void enableHealthCheck(String enableHealthPath) throws KeeperException, InterruptedException {
+ zk.delete(enableHealthPath, -1);
+ }
+
+ public boolean enableHealthCheck() {
+ try {
+ return null == zk.exists(conf.getEnableHealthPath(), false);
+ } catch (Exception e) {
Review comment:
do not catch Exception.
in this case it is better to let the error bubble out
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/health/SwitchOfHealthCheckCommand.java
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.tools.cli.commands.health;
+
+import com.beust.jcommander.Parameter;
+import com.google.common.util.concurrent.UncheckedExecutionException;
+import lombok.Setter;
+import lombok.experimental.Accessors;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.meta.LedgerUnderreplicationManager;
+import org.apache.bookkeeper.meta.MetadataDrivers;
+import org.apache.bookkeeper.meta.exceptions.MetadataException;
+import org.apache.bookkeeper.meta.zk.ZKMetadataBookieDriver;
+import org.apache.bookkeeper.meta.zk.ZKMetadataDriverBase;
+import org.apache.bookkeeper.replication.ReplicationException;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.tools.framework.CliFlags;
+import org.apache.bookkeeper.tools.framework.CliSpec;
+import org.apache.commons.configuration.ConfigurationException;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.concurrent.ExecutionException;
+
+
+/**
+ * Command to enable or disable auto recovery in the cluster.
+ */
+public class SwitchOfHealthCheckCommand extends BookieCommand<SwitchOfHealthCheckCommand.HealthCheckFlags> {
+
+ static final Logger LOG = LoggerFactory.getLogger(SwitchOfHealthCheckCommand.class);
+
+ private static final String NAME = "switch";
+ private static final String DESC = "Enable or disable health check in the cluster. Default is enable.";
+
+ public SwitchOfHealthCheckCommand() {
+ this(new HealthCheckFlags());
+ }
+
+ private SwitchOfHealthCheckCommand(HealthCheckFlags flags) {
+ super(CliSpec.<HealthCheckFlags>newBuilder()
+ .withName(NAME).withDescription(DESC)
+ .withFlags(flags).build());
+ }
+
+ /**
+ * Flags for health check command.
+ */
+ @Accessors(fluent = true)
+ @Setter
+ public static class HealthCheckFlags extends CliFlags {
+
+ @Parameter(names = { "-d", "--disable" }, description = "Enable or disable health check.")
Review comment:
usually it is better to have "positive" options, like "--enable"
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/health/SwitchOfHealthCheckCommand.java
##########
@@ -0,0 +1,128 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.tools.cli.commands.health;
+
+import com.beust.jcommander.Parameter;
+import com.google.common.util.concurrent.UncheckedExecutionException;
+import lombok.Setter;
+import lombok.experimental.Accessors;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.meta.LedgerUnderreplicationManager;
+import org.apache.bookkeeper.meta.MetadataDrivers;
+import org.apache.bookkeeper.meta.exceptions.MetadataException;
+import org.apache.bookkeeper.meta.zk.ZKMetadataBookieDriver;
+import org.apache.bookkeeper.meta.zk.ZKMetadataDriverBase;
+import org.apache.bookkeeper.replication.ReplicationException;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.tools.framework.CliFlags;
+import org.apache.bookkeeper.tools.framework.CliSpec;
+import org.apache.commons.configuration.ConfigurationException;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.concurrent.ExecutionException;
+
+
+/**
+ * Command to enable or disable auto recovery in the cluster.
+ */
+public class SwitchOfHealthCheckCommand extends BookieCommand<SwitchOfHealthCheckCommand.HealthCheckFlags> {
+
+ static final Logger LOG = LoggerFactory.getLogger(SwitchOfHealthCheckCommand.class);
+
+ private static final String NAME = "switch";
+ private static final String DESC = "Enable or disable health check in the cluster. Default is enable.";
+
+ public SwitchOfHealthCheckCommand() {
+ this(new HealthCheckFlags());
+ }
+
+ private SwitchOfHealthCheckCommand(HealthCheckFlags flags) {
+ super(CliSpec.<HealthCheckFlags>newBuilder()
+ .withName(NAME).withDescription(DESC)
+ .withFlags(flags).build());
+ }
+
+ /**
+ * Flags for health check command.
+ */
+ @Accessors(fluent = true)
+ @Setter
+ public static class HealthCheckFlags extends CliFlags {
+
+ @Parameter(names = { "-d", "--disable" }, description = "Enable or disable health check.")
+ private boolean disable;
+
+ @Parameter(names = {"-s", "--status"}, description = "Check the health check status.")
+ private boolean status;
+
+ }
+
+ @Override
+ public boolean apply(ServerConfiguration conf, HealthCheckFlags cmdFlags) {
+ try {
+ return handler(conf, cmdFlags);
+ } catch (MetadataException | ExecutionException e) {
+ throw new UncheckedExecutionException(e.getMessage(), e);
+ }
+ }
+
+ private boolean handler(ServerConfiguration conf, HealthCheckFlags flags)
+ throws MetadataException, ExecutionException {
+
+ MetadataDrivers.runFunctionWithMetadataBookieDriver(conf, driver -> {
+ try {
+ String enableHealthPath = conf.getEnableHealthPath();
+
+ if(!(driver instanceof ZKMetadataBookieDriver)){
Review comment:
please remove this line, you can add a dummy implementation in the MetadataBookieDriver (using "default" methods)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-996333456
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773826593
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
+ deleteResult.completeExceptionally(e);
+ }
+ return deleteResult;
+ }
+
+ public CompletableFuture<Boolean> isEnableHealthCheck() {
+ CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
+ try {
+ boolean isEnable = (null == zk.exists(getEnableHealthPath(), false));
+ enableResult.complete(isEnable);
+ } catch (Exception e) {
+ enableResult.completeExceptionally(e);
+ }
+ return enableResult;
+ }
+
+
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(conf);
Review comment:
OK, I will fixed
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773815069
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
+ deleteResult.completeExceptionally(e);
+ }
+ return deleteResult;
+ }
+
+ public CompletableFuture<Boolean> isEnableHealthCheck() {
+ CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
+ try {
+ boolean isEnable = (null == zk.exists(getEnableHealthPath(), false));
+ enableResult.complete(isEnable);
+ } catch (Exception e) {
+ enableResult.completeExceptionally(e);
+ }
+ return enableResult;
+ }
+
+
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(conf);
Review comment:
I meant the whole zk path, including /enableHealthCheck, so you don't need computation in the "hot" path
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773806832
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
Review comment:
like this?
public CompletableFuture<Void> disableHealthCheck() {
CompletableFuture<Void> createResult = new CompletableFuture<>();
try {
zk.create(getEnableHealthPath(), BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
createResult.complete(null);
} catch (Exception e) {
createResult.completeExceptionally(e);
}
return createResult;
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774868763
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(enableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
@lordcheng10 I see what you mean. Thanks.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1000671763
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773717253
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
Review comment:
like this ?
public CompletableFuture<Void> enableHealthCheck() {
CompletableFuture<Void> deleteResult = new CompletableFuture<>();
try {
zk.delete(getEnableHealthPath(), -1);
deleteResult.complete(null);
} catch (KeeperException.NoNodeException noNodeException) {
deleteResult.complete(null);
} catch (Exception e) {
deleteResult.completeExceptionally(e);
}
return deleteResult;
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774944482
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
like this?
public CompletableFuture<Void> disableHealthCheck() {
CompletableFuture<Void> createResult = new CompletableFuture<>();
zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls,
CreateMode.PERSISTENT, new AsyncCallback.StringCallback() {
@Override
public void processResult(int rc, String path, Object ctx, String name) {
if (KeeperException.Code.OK.intValue() == rc) {
createResult.complete(null);
} else if (KeeperException.Code.NODEEXISTS.intValue() == rc) {
log.debug("health check already disable!");
createResult.complete(null);
} else {
createResult.completeExceptionally(KeeperException.create(KeeperException.Code.get(rc), path));
}
}
}, null);
return createResult;
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773833078
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
Review comment:
OK ，I will fix
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
Review comment:
OK, I will fix
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r777217174
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,29 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
Review comment:
Why do we need this method on the bookie driver?
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataClientDriver.java
##########
@@ -103,4 +105,28 @@ LedgerManagerFactory getLedgerManagerFactory()
* listener listening on metadata client session states.
*/
void setSessionStateListener(SessionStateListener sessionStateListener);
+
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
+ return FutureUtils.Void();
Review comment:
I thought more about this.
Probably it is better to report an error by default in this method for drivers that do not support this feature.
Otherwise people will think that they disabled the health check but they actually didn't
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -614,8 +614,24 @@ public void safeRun() {
}
}
- void checkForFaultyBookies() {
+ void checkForFaultyBookies() {
List<BookieId> faultyBookies = bookieClient.getFaultyBookies();
+ if (faultyBookies.isEmpty()) {
+ return;
+ }
+
+ boolean isEnable = true;
+ try {
+ isEnable = metadataDriver.isHealthCheckEnabled().get();
+ } catch (Exception e) {
Review comment:
Please don't catch raw Exception.
If reading from ZK fails it is likely that we won't be able to move forward in the following lines, so it is better to easly exist.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r777226086
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataClientDriver.java
##########
@@ -103,4 +105,28 @@ LedgerManagerFactory getLedgerManagerFactory()
* listener listening on metadata client session states.
*/
void setSessionStateListener(SessionStateListener sessionStateListener);
+
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
+ return FutureUtils.Void();
Review comment:
like this?
default CompletableFuture<Void> disableHealthCheck() {
new Exception("Not support disableHealthCheck");
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1004045083
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1012880786
@pkumar-singh @zymap @dlg99 PTAL,thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1013638649
Thank you for your suggestion,According to your suggestion I have made the corresponding changes，please review again！@dlg99
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1012325091
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r771080047
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
##########
@@ -525,6 +527,15 @@ public void setZkLedgersRootPath(String zkLedgersPath) {
setProperty(ZK_LEDGERS_ROOT_PATH, zkLedgersPath);
}
+ /**
+ * @return zk enable health path
+ * */
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(this);
+ String ledgerParentPath = zkLedgersRootPath.substring(0, zkLedgersRootPath.lastIndexOf("/ledgers"));
+ return String.format("%s/%s", ledgerParentPath, "enableHealthCheck");
Review comment:
like this:
@zymap
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r771136274
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataClientDriver.java
##########
@@ -103,4 +103,25 @@ LedgerManagerFactory getLedgerManagerFactory()
* listener listening on metadata client session states.
*/
void setSessionStateListener(SessionStateListener sessionStateListener);
+
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default boolean isEnableHealthCheck() {
+ return true;
+ }
+
+ /**
+ * Disable health check.
+ */
+ default void disableHealthCheck(String enableHealthPath) {
Review comment:
same here
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
##########
@@ -525,6 +527,15 @@ public void setZkLedgersRootPath(String zkLedgersPath) {
setProperty(ZK_LEDGERS_ROOT_PATH, zkLedgersPath);
}
+ /**
+ * @return zk enable health path
+ * */
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(this);
Review comment:
we cannot refer to ZKMetadataDriverBase here, that is a base class.
We should move this method to the ZK implementation
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +260,21 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ @SneakyThrows
Review comment:
@SneakyThrows is usually a bad thing as it hides potential problems
we use Checked exceptions in order to tell to the developer that things can go wrong and failures should be handled
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +68,27 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default boolean isEnableHealthCheck() {
+ return true;
+ }
+
+ /**
+ * Disable health check.
+ */
+ default void disableHealthCheck(String enableHealthPath) {
Review comment:
the enableHealthPath depends on the driver implementation
I believe that there is no need to pass the path here,
you can compute in the ZK driver
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +68,27 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default boolean isEnableHealthCheck() {
+ return true;
+ }
+
+ /**
+ * Disable health check.
+ */
+ default void disableHealthCheck(String enableHealthPath) {
+ }
+
+ /**
+ * Enable health check.
+ */
+ default void enableHealthCheck(String enableHealthPath) {
Review comment:
same here
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +68,27 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default boolean isEnableHealthCheck() {
+ return true;
+ }
+
+ /**
+ * Disable health check.
+ */
+ default void disableHealthCheck(String enableHealthPath) {
Review comment:
also, this method would throw exceptions and also all the drivers are async.
I suggest to change the signature to
`CompletableFuture<Void> disableHealthCheck()`
here and for the other new methods
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r771914881
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
##########
@@ -525,6 +527,15 @@ public void setZkLedgersRootPath(String zkLedgersPath) {
setProperty(ZK_LEDGERS_ROOT_PATH, zkLedgersPath);
}
+ /**
+ * @return zk enable health path
+ * */
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(this);
Review comment:
OK, I will move the getEnableHealthPath method implementation to the ZKMetadataDriverBase class.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774939148
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ log.debug("health check already disable!");
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(disableHealthCheckPath, -1);
Review comment:
like this?
public CompletableFuture<Void> disableHealthCheck() {
CompletableFuture<Void> createResult = new CompletableFuture<>();
zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT, new AsyncCallback.StringCallback() {
@Override
public void processResult(int rc, String path, Object ctx, String name) {
if (KeeperException.Code.OK.intValue() == rc) {
createResult.complete(null);
} else if (KeeperException.Code.NODEEXISTS.intValue() == rc) {
log.debug("health check already disable!");
createResult.complete(null);
} else {
createResult.completeExceptionally(KeeperException.create(KeeperException.Code.get(rc), path));
}
}
}, null);
return createResult;
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1000888299
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773717331
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -614,7 +614,19 @@ public void safeRun() {
}
}
- void checkForFaultyBookies() {
+ void checkForFaultyBookies() {
+ boolean isEnable = true;
+ try {
+ isEnable = metadataDriver.isEnableHealthCheck().get();
+ } catch (Exception e) {
+ LOG.error("call method isEnableHealthCheck failed!", e);
Review comment:
yes, from a user/admin side I'd like to read something like
`can't determine if the bookie health checker is enabled, falling back to the default behavior (checker enabled)` or similar
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-999525016
According to your suggestions, I made the corresponding changes, please review again.
Thanks! @nicoloboschi
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773848090
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +269,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(enableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ log.debug("enableHealthCheckPath {} is existed!", enableHealthCheckPath);
Review comment:
```suggestion
log.debug("health check already disabled");
```
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +269,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(enableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ log.debug("enableHealthCheckPath {} is existed!", enableHealthCheckPath);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(enableHealthCheckPath, -1);
+ deleteResult.complete(null);
+ } catch (KeeperException.NoNodeException noNodeException) {
+ log.debug("enableHealthCheckPath {} is not existed!", enableHealthCheckPath);
Review comment:
```suggestion
log.debug("health check already enabled");
```
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,29 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isEnableHealthCheck() {
Review comment:
```suggestion
default CompletableFuture<Boolean> isHealthCheckEnabled() {
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774837857
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(enableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
@pkumar-singh
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1000652628
I made two changes:
1. rename enableHealthCheckPath to disableHealthCheckPath;
2. 2.Need to call getFaultyBookies method first, this method call can reset errorCounter to 0:
https://github.com/apache/bookkeeper/blob/4dae979c5b35c434105f0c9810c87dd91237bb7e/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieClientImpl.java#L160-L163
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r777224201
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,29 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
Review comment:
This method metadatabookiedriver disablehealthcheck is used in the class switchofhealthcheckcommand。
I guess you want to remove the metadataclientdriver#enablehealthcheck method？
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1004232367
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1007062630
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1008929108
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1014052862
@zymap PTAL,thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r783946154
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -616,6 +617,23 @@ public void safeRun() {
void checkForFaultyBookies() {
List<BookieId> faultyBookies = bookieClient.getFaultyBookies();
+ if (faultyBookies.isEmpty()) {
+ return;
+ }
+
+ boolean isEnable = false;
+ try {
+ isEnable = metadataDriver.isHealthCheckEnabled().get();
+ } catch (InterruptedException e) {
+ LOG.error("isHealthCheckEnabled throw InterruptedException", e);
Review comment:
OK I will fix
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1012880786
@pkumar-singh PTAL,thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r785282997
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -616,6 +617,24 @@ public void safeRun() {
void checkForFaultyBookies() {
List<BookieId> faultyBookies = bookieClient.getFaultyBookies();
+ if (faultyBookies.isEmpty()) {
+ return;
+ }
+
+ boolean isEnable = false;
Review comment:
OK I will fix
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +270,62 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls,
+ CreateMode.PERSISTENT, new AsyncCallback.StringCallback() {
+ @Override
+ public void processResult(int rc, String path, Object ctx, String name) {
+ if (KeeperException.Code.OK.intValue() == rc) {
+ createResult.complete(null);
+ } else if (KeeperException.Code.NODEEXISTS.intValue() == rc) {
+ log.debug("health check already disable!");
Review comment:
OK I will fix
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r777224201
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,29 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
Review comment:
This method metadatabookiedriver disablehealthcheck is used in the class SwitchOfHealthCheckCommand。
@eolivelli
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r777290499
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -614,8 +614,24 @@ public void safeRun() {
}
}
- void checkForFaultyBookies() {
+ void checkForFaultyBookies() {
List<BookieId> faultyBookies = bookieClient.getFaultyBookies();
+ if (faultyBookies.isEmpty()) {
+ return;
+ }
+
+ boolean isEnable = true;
+ try {
+ isEnable = metadataDriver.isHealthCheckEnabled().get();
+ } catch (Exception e) {
Review comment:
OK, I will fixed
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1003884481
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-998668409
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1008539592
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1011953730
1.catch "ExecutionException, InterruptedException" ；
2.remove exception on safeRun;
3.return CompletableFuture should not throw Exceptions;
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1004469039
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 removed a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 removed a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1013797425
@zymap PTAL,Thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1014067461
@dlg99 @eolivelli @pkumar-singh @zymap This PR has been open for a long time. If there is no problem, can you merge it for me? thank you!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] zymap commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r771058033
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
##########
@@ -525,6 +527,15 @@ public void setZkLedgersRootPath(String zkLedgersPath) {
setProperty(ZK_LEDGERS_ROOT_PATH, zkLedgersPath);
}
+ /**
+ * @return zk enable health path
+ * */
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(this);
+ String ledgerParentPath = zkLedgersRootPath.substring(0, zkLedgersRootPath.lastIndexOf("/ledgers"));
+ return String.format("%s/%s", ledgerParentPath, "enableHealthCheck");
Review comment:
why not put the health path into the `/ledgers`?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-996067673
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-995767516
With this command, you can turn off the health check of the client, so as to avoid isolating the bookie when upgrading the bookie:
enable health check: sh bkctl healthCheck switch -e
disable helth check: sh bkctl healthCheck switch
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 removed a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 removed a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-997392339
> Please address Enrico's comments, otherwise LGTM.
Following Enrico’s suggestion, I modified it accordingly
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773717253
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
Review comment:
like this ? @nicoloboschi
public CompletableFuture<Void> enableHealthCheck() {
CompletableFuture<Void> deleteResult = new CompletableFuture<>();
try {
zk.delete(getEnableHealthPath(), -1);
deleteResult.complete(null);
} catch (KeeperException.NoNodeException noNodeException) {
deleteResult.complete(null);
} catch (Exception e) {
deleteResult.completeExceptionally(e);
}
return deleteResult;
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773638556
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,36 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isEnableHealthCheck() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
+ return FutureUtils.Void();
+ }
+
+ /**
+ * Enable health check.
+ */
+ default CompletableFuture<Void> enableHealthCheck() {
+ return FutureUtils.Void();
+ }
+
+ /**
+ * @return disable health check path
+ */
+ default String getEnableHealthPath() {
Review comment:
this "path" looks like a ZK configuration and not an abstraction, I'd remove it from the interface
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -614,7 +614,19 @@ public void safeRun() {
}
}
- void checkForFaultyBookies() {
+ void checkForFaultyBookies() {
+ boolean isEnable = true;
+ try {
+ isEnable = metadataDriver.isEnableHealthCheck().get();
+ } catch (Exception e) {
+ LOG.error("call method isEnableHealthCheck failed!", e);
Review comment:
in case of error, we could explain in the log that the value is treated as false
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
Review comment:
KeeperException.NoNodeException is not a real error here (because it's already enabled)
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
Review comment:
what about KeeperException.NodeExistsException ? we should ignore this sub-exception because it's not a real problem
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
Review comment:
BookKeeperConstants.EMPTY_BYTE_ARRAY
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
Review comment:
this.acls ?
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
+ deleteResult.completeExceptionally(e);
+ }
+ return deleteResult;
+ }
+
+ public CompletableFuture<Boolean> isEnableHealthCheck() {
+ CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
+ try {
+ boolean isEnable = (null == zk.exists(getEnableHealthPath(), false));
+ enableResult.complete(isEnable);
+ } catch (Exception e) {
+ enableResult.completeExceptionally(e);
+ }
+ return enableResult;
+ }
+
+
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(conf);
Review comment:
we should cache this return value in the `initialize` method. we don't need to recalculate it everytime
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
+ deleteResult.completeExceptionally(e);
+ }
+ return deleteResult;
+ }
+
+ public CompletableFuture<Boolean> isEnableHealthCheck() {
+ CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
+ try {
+ boolean isEnable = (null == zk.exists(getEnableHealthPath(), false));
+ enableResult.complete(isEnable);
+ } catch (Exception e) {
+ enableResult.completeExceptionally(e);
+ }
+ return enableResult;
+ }
+
+
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(conf);
+ String ledgerParentPath = zkLedgersRootPath.substring(0, zkLedgersRootPath.lastIndexOf("/ledgers"));
+ return String.format("%s/%s", ledgerParentPath, "enableHealthCheck");
Review comment:
we should add the ZK node name to the BookKeeperConstants class
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1000664101
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774942785
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
like this?
public CompletableFuture<Void> disableHealthCheck() {
CompletableFuture<Void> createResult = new CompletableFuture<>();
zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT, new AsyncCallback.StringCallback() {
@Override
public void processResult(int rc, String path, Object ctx, String name) {
if (KeeperException.Code.OK.intValue() == rc) {
createResult.complete(null);
} else if (KeeperException.Code.NODEEXISTS.intValue() == rc) {
log.debug("health check already disable!");
createResult.complete(null);
} else {
createResult.completeExceptionally(KeeperException.create(KeeperException.Code.get(rc), path));
}
}
}, null);
return createResult;
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1000741964
According to your suggestion, I used the async API of ZooKeeper to rewrite 3 methods: disableHealthCheck(), enableHealthCheck() and isHealthCheckEnabled();
Please review again, thank you ! @eolivelli
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-999974563
PTAL，thanks! @dlg99 @eolivelli
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1005425349
Please review again, thank you ! @eolivelli
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 removed a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 removed a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1008222502
@pkumar-singh PTAL,thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1007384952
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774830125
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(enableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
We will restart bookie frequently while upgrading the BookKeeper cluster. Do we need disabled Health Check every time we stop Bookie？
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774950416
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ log.debug("health check already disable!");
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(disableHealthCheckPath, -1);
+ deleteResult.complete(null);
+ } catch (KeeperException.NoNodeException noNodeException) {
+ log.debug("health check already enabled!");
+ deleteResult.complete(null);
+ } catch (Exception e) {
+ deleteResult.completeExceptionally(e);
+ }
+ return deleteResult;
+ }
+
+ public CompletableFuture<Boolean> isHealthCheckEnabled() {
+ CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
+ try {
+ boolean isEnable = (null == zk.exists(disableHealthCheckPath, false));
Review comment:
like this?
public CompletableFuture<Void> disableHealthCheck() {
CompletableFuture<Void> createResult = new CompletableFuture<>();
zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls,
CreateMode.PERSISTENT, new AsyncCallback.StringCallback() {
public void processResult(int rc, String path, Object ctx, String name) {
if (KeeperException.Code.OK.intValue() == rc) {
createResult.complete(null);
} else if (KeeperException.Code.NODEEXISTS.intValue() == rc) {
log.debug("health check already disable!");
createResult.complete(null);
} else {
createResult.completeExceptionally(KeeperException.create(KeeperException.Code.get(rc), path));
}
}
}, null);
return createResult;
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774950876
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (KeeperException.NodeExistsException nodeExistsException) {
+ log.debug("health check already disable!");
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(disableHealthCheckPath, -1);
Review comment:
public CompletableFuture<Void> enableHealthCheck() {
CompletableFuture<Void> deleteResult = new CompletableFuture<>();
zk.delete(disableHealthCheckPath, -1, new AsyncCallback.VoidCallback() {
@Override
public void processResult(int rc, String path, Object ctx) {
if (KeeperException.Code.OK.intValue() == rc) {
deleteResult.complete(null);
} else if (KeeperException.Code.NONODE.intValue() == rc) {
log.debug("health check already enabled!");
deleteResult.complete(null);
} else {
deleteResult.completeExceptionally(KeeperException.create(KeeperException.Code.get(rc), path));
}
}
}, null);
return deleteResult;
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1012658110
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1007384952
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 removed a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 removed a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1005425349
Please review again, thank you ! @eolivelli
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1004469219
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1010398012
@michaeljmarshall PTAL，thanks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1014054525
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1014064847
@zymap This PR has been open for a long time. If there is no problem, can you merge it for me? thank you
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1010938126
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1011953730
Based on your suggestion, I made the following changes:
1.catch "ExecutionException, InterruptedException" ；
2.remove exception on safeRun;
3.return CompletableFuture should not throw Exceptions;
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
dlg99 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r785038628
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -616,6 +617,24 @@ public void safeRun() {
void checkForFaultyBookies() {
List<BookieId> faultyBookies = bookieClient.getFaultyBookies();
+ if (faultyBookies.isEmpty()) {
+ return;
+ }
+
+ boolean isEnable = false;
Review comment:
nit: `isEnabled` (here and in other places)
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +270,62 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ zk.create(disableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls,
+ CreateMode.PERSISTENT, new AsyncCallback.StringCallback() {
+ @Override
+ public void processResult(int rc, String path, Object ctx, String name) {
+ if (KeeperException.Code.OK.intValue() == rc) {
+ createResult.complete(null);
+ } else if (KeeperException.Code.NODEEXISTS.intValue() == rc) {
+ log.debug("health check already disable!");
Review comment:
```suggestion
log.debug("health check already disabled!");
```
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/health/SwitchOfHealthCheckCommand.java
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.tools.cli.commands.health;
+
+import com.beust.jcommander.Parameter;
+import com.google.common.util.concurrent.UncheckedExecutionException;
+import java.util.concurrent.ExecutionException;
+import lombok.Setter;
+import lombok.experimental.Accessors;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.meta.MetadataDrivers;
+import org.apache.bookkeeper.meta.exceptions.MetadataException;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.tools.framework.CliFlags;
+import org.apache.bookkeeper.tools.framework.CliSpec;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+
+/**
+ * Command to enable or disable health check in the cluster.
+ */
+public class SwitchOfHealthCheckCommand extends BookieCommand<SwitchOfHealthCheckCommand.HealthCheckFlags> {
+
+ static final Logger LOG = LoggerFactory.getLogger(SwitchOfHealthCheckCommand.class);
+
+ private static final String NAME = "switch";
+ private static final String DESC = "Enable or disable health check in the cluster. Default is enable.";
Review comment:
```suggestion
private static final String DESC = "Enables or disables health check in the cluster. Default is enabled.";
```
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/health/SwitchOfHealthCheckCommand.java
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.tools.cli.commands.health;
+
+import com.beust.jcommander.Parameter;
+import com.google.common.util.concurrent.UncheckedExecutionException;
+import java.util.concurrent.ExecutionException;
+import lombok.Setter;
+import lombok.experimental.Accessors;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.meta.MetadataDrivers;
+import org.apache.bookkeeper.meta.exceptions.MetadataException;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.tools.framework.CliFlags;
+import org.apache.bookkeeper.tools.framework.CliSpec;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+
+/**
+ * Command to enable or disable health check in the cluster.
+ */
+public class SwitchOfHealthCheckCommand extends BookieCommand<SwitchOfHealthCheckCommand.HealthCheckFlags> {
+
+ static final Logger LOG = LoggerFactory.getLogger(SwitchOfHealthCheckCommand.class);
+
+ private static final String NAME = "switch";
+ private static final String DESC = "Enable or disable health check in the cluster. Default is enable.";
+
+ public SwitchOfHealthCheckCommand() {
+ this(new HealthCheckFlags());
+ }
+
+ private SwitchOfHealthCheckCommand(HealthCheckFlags flags) {
+ super(CliSpec.<HealthCheckFlags>newBuilder()
+ .withName(NAME).withDescription(DESC)
+ .withFlags(flags).build());
+ }
+
+ /**
+ * Flags for health check command.
+ */
+ @Accessors(fluent = true)
+ @Setter
+ public static class HealthCheckFlags extends CliFlags {
+
+ @Parameter(names = { "-e", "--enable" }, description = "Enable or disable health check.")
+ private boolean enable;
+
+ @Parameter(names = {"-s", "--status"}, description = "Check the health check status.")
+ private boolean status;
+
+ }
+
+ @Override
+ public boolean apply(ServerConfiguration conf, HealthCheckFlags cmdFlags) {
+ try {
+ return handler(conf, cmdFlags);
+ } catch (MetadataException | ExecutionException e) {
+ throw new UncheckedExecutionException(e.getMessage(), e);
+ }
+ }
+
+ private boolean handler(ServerConfiguration conf, HealthCheckFlags flags)
+ throws MetadataException, ExecutionException {
+
+ MetadataDrivers.runFunctionWithMetadataBookieDriver(conf, driver -> {
+ try {
+ boolean isEnable = driver.isHealthCheckEnabled().get();
+
+ if (flags.status) {
+ LOG.info("EnableHealthCheck is " + (isEnable ? "enabled." : "disabled."));
+ return null;
+ }
+
+ if (flags.enable) {
+ if (isEnable) {
+ LOG.warn("HealthCheck already enable. Doing nothing");
+ } else {
+ LOG.info("Enable HealthCheck");
+ driver.enableHealthCheck().get();
+ }
+ } else {
+ if (!isEnable) {
+ LOG.warn("HealthCheck already disable. Doing nothing");
Review comment:
"already disabled"
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/health/SwitchOfHealthCheckCommand.java
##########
@@ -0,0 +1,116 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.bookkeeper.tools.cli.commands.health;
+
+import com.beust.jcommander.Parameter;
+import com.google.common.util.concurrent.UncheckedExecutionException;
+import java.util.concurrent.ExecutionException;
+import lombok.Setter;
+import lombok.experimental.Accessors;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.meta.MetadataDrivers;
+import org.apache.bookkeeper.meta.exceptions.MetadataException;
+import org.apache.bookkeeper.tools.cli.helpers.BookieCommand;
+import org.apache.bookkeeper.tools.framework.CliFlags;
+import org.apache.bookkeeper.tools.framework.CliSpec;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+
+/**
+ * Command to enable or disable health check in the cluster.
+ */
+public class SwitchOfHealthCheckCommand extends BookieCommand<SwitchOfHealthCheckCommand.HealthCheckFlags> {
+
+ static final Logger LOG = LoggerFactory.getLogger(SwitchOfHealthCheckCommand.class);
+
+ private static final String NAME = "switch";
+ private static final String DESC = "Enable or disable health check in the cluster. Default is enable.";
+
+ public SwitchOfHealthCheckCommand() {
+ this(new HealthCheckFlags());
+ }
+
+ private SwitchOfHealthCheckCommand(HealthCheckFlags flags) {
+ super(CliSpec.<HealthCheckFlags>newBuilder()
+ .withName(NAME).withDescription(DESC)
+ .withFlags(flags).build());
+ }
+
+ /**
+ * Flags for health check command.
+ */
+ @Accessors(fluent = true)
+ @Setter
+ public static class HealthCheckFlags extends CliFlags {
+
+ @Parameter(names = { "-e", "--enable" }, description = "Enable or disable health check.")
+ private boolean enable;
+
+ @Parameter(names = {"-s", "--status"}, description = "Check the health check status.")
+ private boolean status;
+
+ }
+
+ @Override
+ public boolean apply(ServerConfiguration conf, HealthCheckFlags cmdFlags) {
+ try {
+ return handler(conf, cmdFlags);
+ } catch (MetadataException | ExecutionException e) {
+ throw new UncheckedExecutionException(e.getMessage(), e);
+ }
+ }
+
+ private boolean handler(ServerConfiguration conf, HealthCheckFlags flags)
+ throws MetadataException, ExecutionException {
+
+ MetadataDrivers.runFunctionWithMetadataBookieDriver(conf, driver -> {
+ try {
+ boolean isEnable = driver.isHealthCheckEnabled().get();
+
+ if (flags.status) {
+ LOG.info("EnableHealthCheck is " + (isEnable ? "enabled." : "disabled."));
+ return null;
+ }
+
+ if (flags.enable) {
+ if (isEnable) {
+ LOG.warn("HealthCheck already enable. Doing nothing");
Review comment:
"already 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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1011988321
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1002387478
@eolivelli PTAL,thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1003900249
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r777224201
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,29 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
Review comment:
This method metadatabookiedriver disablehealthcheck is used in the class switchofhealthcheckcommand。
I guess you want to remove the metadataclientdriver#enablehealthcheck method？
@eolivelli
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1004000131
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1004244821
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-996011752
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r771120360
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
##########
@@ -525,6 +527,15 @@ public void setZkLedgersRootPath(String zkLedgersPath) {
setProperty(ZK_LEDGERS_ROOT_PATH, zkLedgersPath);
}
+ /**
+ * @return zk enable health path
+ * */
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(this);
+ String ledgerParentPath = zkLedgersRootPath.substring(0, zkLedgersRootPath.lastIndexOf("/ledgers"));
+ return String.format("%s/%s", ledgerParentPath, "enableHealthCheck");
Review comment:
Here, the string is divided according to the configured metadata service URI, for example:
metadataServiceUri=zk+hie rarchical://localhost:2181 ;/ bookie_ ss_ share/ledgers
Then the intercepted string path is: / bookie_ ss_ share，Like this:
@zymap
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773801951
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
+ deleteResult.completeExceptionally(e);
+ }
+ return deleteResult;
+ }
+
+ public CompletableFuture<Boolean> isEnableHealthCheck() {
+ CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
+ try {
+ boolean isEnable = (null == zk.exists(getEnableHealthPath(), false));
+ enableResult.complete(isEnable);
+ } catch (Exception e) {
+ enableResult.completeExceptionally(e);
+ }
+ return enableResult;
+ }
+
+
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(conf);
Review comment:
The same directory seems to be defined, can I use ledgerRootPath directly?
https://github.com/apache/bookkeeper/blob/370d785ff674841943fa8cddee1964408e826f5b/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java#L143~L145
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +262,47 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(getEnableHealthPath(), new byte[0], ZkUtils.getACLs(conf), CreateMode.PERSISTENT);
+ createResult.complete(null);
+ } catch (Exception e) {
+ createResult.completeExceptionally(e);
+ }
+ return createResult;
+ }
+
+ public CompletableFuture<Void> enableHealthCheck() {
+ CompletableFuture<Void> deleteResult = new CompletableFuture<>();
+
+ try {
+ zk.delete(getEnableHealthPath(), -1);
+ deleteResult.complete(null);
+ } catch (Exception e) {
+ deleteResult.completeExceptionally(e);
+ }
+ return deleteResult;
+ }
+
+ public CompletableFuture<Boolean> isEnableHealthCheck() {
+ CompletableFuture<Boolean> enableResult = new CompletableFuture<>();
+ try {
+ boolean isEnable = (null == zk.exists(getEnableHealthPath(), false));
+ enableResult.complete(isEnable);
+ } catch (Exception e) {
+ enableResult.completeExceptionally(e);
+ }
+ return enableResult;
+ }
+
+
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(conf);
Review comment:
The same directory seems to be defined, can I use ledgerRootPath directly?
https://github.com/apache/bookkeeper/blob/370d785ff674841943fa8cddee1964408e826f5b/bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java#L143-L145
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773933858
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -230,6 +238,9 @@ protected void initialize(AbstractConfiguration<?> conf,
this.ownZKHandle = true;
}
+ int endIndex = ledgersRootPath.lastIndexOf(DEFAULT_ZK_LEDGERS_ROOT_PATH);
+ String enableHealthCheckParentPath = ledgersRootPath.substring(0, endIndex == -1 ? 0 : endIndex);
+ enableHealthCheckPath = String.format("%s/%s", enableHealthCheckParentPath, ENABLE_HEALTH_CHECK);
Review comment:
why not just
`final String enableHealthCheckPath = ledgersRootPath + "/" + ENABLE_HEALTH_CHECK;`
?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1009953474
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1010631749
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r783773417
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -606,16 +607,26 @@ void scheduleBookieHealthCheckIfEnabled(ClientConfiguration conf) {
scheduler.scheduleAtFixedRate(new SafeRunnable() {
@Override
- public void safeRun() {
+ public void safeRun() throws ExecutionException, InterruptedException {
checkForFaultyBookies();
}
}, conf.getBookieHealthCheckIntervalSeconds(), conf.getBookieHealthCheckIntervalSeconds(),
TimeUnit.SECONDS);
}
}
- void checkForFaultyBookies() {
+ void checkForFaultyBookies() throws ExecutionException, InterruptedException {
List<BookieId> faultyBookies = bookieClient.getFaultyBookies();
+ if (faultyBookies.isEmpty()) {
+ return;
+ }
+
+ boolean isEnable = metadataDriver.isHealthCheckEnabled().get();
Review comment:
like this?
boolean isEnable = false;
try {
isEnable = metadataDriver.isHealthCheckEnabled().get();
} catch (InterruptedException e) {
LOG.error("isHealthCheckEnabled throw InterruptedException", e);
} catch (ExecutionException e) {
LOG.error("isHealthCheckEnabled throw ExecutionException", e);
}
if (!isEnable) {
LOG.info("Health checks is currently disabled!");
return;
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r783773116
##########
File path: bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/SafeRunnable.java
##########
@@ -39,7 +40,7 @@ default void run() {
}
}
- void safeRun();
+ void safeRun() throws ExecutionException, InterruptedException;
Review comment:
Also, I removed ExecutionException, InterruptedException on method checkForFaultyBookies and safeRun
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r783772215
##########
File path: bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/SafeRunnable.java
##########
@@ -39,7 +40,7 @@ default void run() {
}
}
- void safeRun();
+ void safeRun() throws ExecutionException, InterruptedException;
Review comment:
like this?
boolean isEnable = false;
try {
isEnable = metadataDriver.isHealthCheckEnabled().get();
} catch (InterruptedException e) {
LOG.error("isHealthCheckEnabled throw InterruptedException", e);
} catch (ExecutionException e) {
LOG.error("isHealthCheckEnabled throw ExecutionException", e);
}
if (!isEnable) {
LOG.info("Health checks is currently disabled!");
return;
}
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r783943968
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -616,6 +617,23 @@ public void safeRun() {
void checkForFaultyBookies() {
List<BookieId> faultyBookies = bookieClient.getFaultyBookies();
+ if (faultyBookies.isEmpty()) {
+ return;
+ }
+
+ boolean isEnable = false;
+ try {
+ isEnable = metadataDriver.isHealthCheckEnabled().get();
+ } catch (InterruptedException e) {
+ LOG.error("isHealthCheckEnabled throw InterruptedException", e);
Review comment:
here, please add `Thread.currentThread().interrupt();`
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -616,6 +617,23 @@ public void safeRun() {
void checkForFaultyBookies() {
List<BookieId> faultyBookies = bookieClient.getFaultyBookies();
+ if (faultyBookies.isEmpty()) {
+ return;
+ }
+
+ boolean isEnable = false;
+ try {
+ isEnable = metadataDriver.isHealthCheckEnabled().get();
+ } catch (InterruptedException e) {
+ LOG.error("isHealthCheckEnabled throw InterruptedException", e);
Review comment:
`LOG.error("Cannot verify if healthcheck is enabled", e);`
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -616,6 +617,23 @@ public void safeRun() {
void checkForFaultyBookies() {
List<BookieId> faultyBookies = bookieClient.getFaultyBookies();
+ if (faultyBookies.isEmpty()) {
+ return;
+ }
+
+ boolean isEnable = false;
+ try {
+ isEnable = metadataDriver.isHealthCheckEnabled().get();
+ } catch (InterruptedException e) {
+ LOG.error("isHealthCheckEnabled throw InterruptedException", e);
+ } catch (ExecutionException e) {
+ LOG.error("isHealthCheckEnabled throw ExecutionException", e);
Review comment:
`LOG.error("Cannot verify if healthcheck is enabled", e.getCause());`
we can log "e.getCause()" because ExecutionException is always a wrapper for another problem
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1013800226
@zymap PTAL,thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1013797425
@zymap PTAL,Thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1014064847
@zymap This PR has been open for a long time. If there is no problem, can you merge it for me? thank you!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1014062244
@zymap If you have time, please help me review it, thank you!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 removed a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 removed a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1013800226
@zymap PTAL,thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 removed a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 removed a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1014052862
@zymap PTAL,thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1014124901
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-995762267
PTAL, thanks ! @nicoloboschi @dlg99 @eolivelli @Shoothzj
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r770738786
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +261,30 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public boolean isEnableHealthCheck() {
+ try {
+ return null == zk.exists(conf.getEnableHealthPath(), false);
+ } catch (Exception e) {
+ return true;
+ }
+ }
+
+ public void disableHealthCheck(String enableHealthPath) throws Exception{
+ zk.create(enableHealthPath, new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
+ }
+
+ public void enableHealthCheck(String enableHealthPath) throws KeeperException, InterruptedException {
+ zk.delete(enableHealthPath, -1);
+ }
+
+ public boolean enableHealthCheck() {
+ try {
+ return null == zk.exists(conf.getEnableHealthPath(), false);
+ } catch (Exception e) {
Review comment:
You are right
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r770738562
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -614,7 +615,19 @@ public void safeRun() {
}
}
+ boolean enableHealthCheck() {
+ if (metadataDriver instanceof ZKMetadataClientDriver) {
Review comment:
You are right
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-996068133
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r770734294
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +261,30 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public boolean isEnableHealthCheck() {
+ try {
+ return null == zk.exists(conf.getEnableHealthPath(), false);
+ } catch (Exception e) {
+ return true;
+ }
+ }
+
+ public void disableHealthCheck(String enableHealthPath) throws Exception{
+ zk.create(enableHealthPath, new byte[0], ZooDefs.Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT);
Review comment:
Do you mean to add an ACL parameter？like:
public void disableHealthCheck(String enableHealthPath, List<ACL> acl)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r771080047
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
##########
@@ -525,6 +527,15 @@ public void setZkLedgersRootPath(String zkLedgersPath) {
setProperty(ZK_LEDGERS_ROOT_PATH, zkLedgersPath);
}
+ /**
+ * @return zk enable health path
+ * */
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(this);
+ String ledgerParentPath = zkLedgersRootPath.substring(0, zkLedgersRootPath.lastIndexOf("/ledgers"));
+ return String.format("%s/%s", ledgerParentPath, "enableHealthCheck");
Review comment:
like 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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r771080047
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/AbstractConfiguration.java
##########
@@ -525,6 +527,15 @@ public void setZkLedgersRootPath(String zkLedgersPath) {
setProperty(ZK_LEDGERS_ROOT_PATH, zkLedgersPath);
}
+ /**
+ * @return zk enable health path
+ * */
+ public String getEnableHealthPath() {
+ String zkLedgersRootPath = ZKMetadataDriverBase.resolveZkLedgersRootPath(this);
+ String ledgerParentPath = zkLedgersRootPath.substring(0, zkLedgersRootPath.lastIndexOf("/ledgers"));
+ return String.format("%s/%s", ledgerParentPath, "enableHealthCheck");
Review comment:
Here, the string is divided according to the configured metadata service URI, for example:
metadataServiceUri=zk+hie rarchical://localhost:2181 ;/ bookie_ ss_ share/ledgers
Then the intercepted string path is: / bookie_ ss_ share，Like this:
@zymap
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-996457062
Please review again，thanks! @zymap @eolivelli
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-998060086
> I did a second pass, please take a look.
>
> I believe that we are on our way.
>
> We should discuss about this change on [dev@bookkeeper.apache.org](mailto:dev@bookkeeper.apache.org) this way more people can see it and interact
I've started a discussion, see
https://lists.apache.org/thread/o80j9zc6gkdbx0jfyhxptjrycs1xjlmd
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r774736078
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -260,6 +268,46 @@ public synchronized LedgerManagerFactory getLedgerManagerFactory()
return lmFactory;
}
+ public CompletableFuture<Void> disableHealthCheck() {
+ CompletableFuture<Void> createResult = new CompletableFuture<>();
+ try {
+ zk.create(enableHealthCheckPath, BookKeeperConstants.EMPTY_BYTE_ARRAY, acls, CreateMode.PERSISTENT);
Review comment:
Should not this be ephemeral? So that when bookie goes away and comes back again, it should come back in health check enabled mode?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773710130
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##########
@@ -614,7 +614,19 @@ public void safeRun() {
}
}
- void checkForFaultyBookies() {
+ void checkForFaultyBookies() {
+ boolean isEnable = true;
+ try {
+ isEnable = metadataDriver.isEnableHealthCheck().get();
+ } catch (Exception e) {
+ LOG.error("call method isEnableHealthCheck failed!", e);
Review comment:
call method isEnableHealthCheck failed，isEnable is treated as true.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773853063
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,29 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isEnableHealthCheck() {
Review comment:
you are right!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r773943436
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
##########
@@ -230,6 +238,9 @@ protected void initialize(AbstractConfiguration<?> conf,
this.ownZKHandle = true;
}
+ int endIndex = ledgersRootPath.lastIndexOf(DEFAULT_ZK_LEDGERS_ROOT_PATH);
+ String enableHealthCheckParentPath = ledgersRootPath.substring(0, endIndex == -1 ? 0 : endIndex);
+ enableHealthCheckPath = String.format("%s/%s", enableHealthCheckParentPath, ENABLE_HEALTH_CHECK);
Review comment:
OK, I will fix
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1006218656
Please review again, thank you ! @eolivelli
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1002886030
ping
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1003878597
> I left my last comments then I believe that we are good to go
Thank you so much for your suggestions. Great idea！
According to your suggestions, I made the following changes:
1. Don't catch raw Exception when call method of metadataDriver.isHealthCheckEnabled().get();
2.Default action in method of disableHealthCheck is throw exception:
default CompletableFuture<Void> disableHealthCheck() throws Exception {
throw new Exception("Not support disableHealthCheck");
}
3.the methods disableHealthCheck and enableHealthCheck in the class org.apache.bookkeeper.meta.MetadataClientDriver are not used and has been removed. org.apache.bookkeeper.meta.MetadataBookieDriver#disableHealthCheck() is called in the class org.apache.bookkeeper .tools.cli.commands.health.SwitchOfHealthCheckCommand, so it cannot be removed。
@eolivelli
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#discussion_r777224201
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MetadataBookieDriver.java
##########
@@ -68,6 +71,29 @@ LedgerManagerFactory getLedgerManagerFactory()
*/
LayoutManager getLayoutManager();
+ /**
+ * Return health check is enable or disable.
+ *
+ * @return true if health check is enable, otherwise false.
+ */
+ default CompletableFuture<Boolean> isHealthCheckEnabled() {
+ return FutureUtils.value(true);
+ }
+
+ /**
+ * Disable health check.
+ */
+ default CompletableFuture<Void> disableHealthCheck() {
Review comment:
This method metadatabookiedriver disablehealthcheck is used in the class SwitchOfHealthCheckCommand。
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1003899857
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1003900309
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 removed a comment on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 removed a comment on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1002387478
@eolivelli PTAL,thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #2947: suport dynamic enable/disable health check
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #2947:
URL: https://github.com/apache/bookkeeper/pull/2947#issuecomment-1008222502
@pkumar-singh PTAL,thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org