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/22 06:57:22 UTC
[GitHub] [bookkeeper] nicoloboschi commented on a change in pull request #2947: suport dynamic enable/disable health check
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