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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
