[GitHub] [helix] junkaixue commented on a change in pull request #1978: Enable HelixManager as an event listener

[GitBox <gi...@apache.org>]

junkaixue commented on a change in pull request #1978:
URL: https://github.com/apache/helix/pull/1978#discussion_r826236968



##########
File path: helix-core/src/main/java/org/apache/helix/cloud/event/helix/AbstractCloudEventCallbackImpl.java
##########
@@ -0,0 +1,46 @@
+package org.apache.helix.cloud.event.helix;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixManager;
+import sun.reflect.generics.reflectiveObjects.NotImplementedException;
+
+public abstract class AbstractCloudEventCallbackImpl {
+
+  public void onPauseDefaultHelixOperation(HelixManager manager, Object eventInfo) {
+    // To be implemented
+    throw new NotImplementedException();

Review comment:
       Why not implement here?

##########
File path: helix-core/src/main/java/org/apache/helix/cloud/event/helix/CloudEventCallbackProperty.java
##########
@@ -0,0 +1,227 @@
+package org.apache.helix.cloud.event.helix;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import org.apache.helix.HelixManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A property for users to customize the behavior of a Helix manager as a cloud event listener
+ * Use this
+ */
+public class CloudEventCallbackProperty {
+  private static final Logger LOG =
+      LoggerFactory.getLogger(CloudEventCallbackProperty.class.getName());
+
+  // The key for user to pass in callback impl class name in the constructor
+  public static final String CALLBACK_IMPL_CLASS_NAME = "callbackImplClassName";
+  private Map<OnPauseOperations, BiConsumer<HelixManager, Object>>
+      _onPauseOperationMap;
+  private Map<OnResumeOperations, BiConsumer<HelixManager, Object>>
+      _onResumeOperationMap;

Review comment:
       Why we need two maps? Why not have two different callback property for onpause/onresume?

##########
File path: helix-core/src/main/java/org/apache/helix/cloud/event/helix/CloudEventCallbackProperty.java
##########
@@ -0,0 +1,227 @@
+package org.apache.helix.cloud.event.helix;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import org.apache.helix.HelixManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A property for users to customize the behavior of a Helix manager as a cloud event listener
+ * Use this
+ */
+public class CloudEventCallbackProperty {
+  private static final Logger LOG =
+      LoggerFactory.getLogger(CloudEventCallbackProperty.class.getName());
+
+  // The key for user to pass in callback impl class name in the constructor
+  public static final String CALLBACK_IMPL_CLASS_NAME = "callbackImplClassName";
+  private Map<OnPauseOperations, BiConsumer<HelixManager, Object>>
+      _onPauseOperationMap;
+  private Map<OnResumeOperations, BiConsumer<HelixManager, Object>>
+      _onResumeOperationMap;
+  private final String _callbackImplClassName;
+
+  /**
+   * Constructor
+   * @param userArgs A map contains information that users pass in
+   */
+  public CloudEventCallbackProperty(Map<String, String> userArgs) {
+    _callbackImplClassName = userArgs.get(CALLBACK_IMPL_CLASS_NAME);
+    _onPauseOperationMap = new ConcurrentHashMap<>();
+    _onResumeOperationMap = new ConcurrentHashMap<>();
+  }
+
+  /**
+   * A collection of types of optional Helix-supported operations
+   */
+  public enum OptionalHelixOperation {
+    MAINTENANCE_MODE
+  }
+
+  /**
+   * A collection of types and positions for user to plugin customized callback
+   */
+  public enum UserDefinedCallbackType {
+    PRE_ON_PAUSE, POST_ON_PAUSE, PRE_ON_RESUME, POST_ON_RESUME,

Review comment:
       NIT: format it.

##########
File path: helix-core/src/main/java/org/apache/helix/cloud/event/helix/CloudEventCallbackProperty.java
##########
@@ -0,0 +1,227 @@
+package org.apache.helix.cloud.event.helix;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import org.apache.helix.HelixManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A property for users to customize the behavior of a Helix manager as a cloud event listener
+ * Use this
+ */
+public class CloudEventCallbackProperty {
+  private static final Logger LOG =
+      LoggerFactory.getLogger(CloudEventCallbackProperty.class.getName());
+
+  // The key for user to pass in callback impl class name in the constructor
+  public static final String CALLBACK_IMPL_CLASS_NAME = "callbackImplClassName";
+  private Map<OnPauseOperations, BiConsumer<HelixManager, Object>>
+      _onPauseOperationMap;
+  private Map<OnResumeOperations, BiConsumer<HelixManager, Object>>
+      _onResumeOperationMap;
+  private final String _callbackImplClassName;
+
+  /**
+   * Constructor
+   * @param userArgs A map contains information that users pass in
+   */
+  public CloudEventCallbackProperty(Map<String, String> userArgs) {
+    _callbackImplClassName = userArgs.get(CALLBACK_IMPL_CLASS_NAME);
+    _onPauseOperationMap = new ConcurrentHashMap<>();
+    _onResumeOperationMap = new ConcurrentHashMap<>();
+  }
+
+  /**
+   * A collection of types of optional Helix-supported operations
+   */
+  public enum OptionalHelixOperation {
+    MAINTENANCE_MODE

Review comment:
       if you already define maintenance mode, how about enable/disable instance? Shall we make it as optional as well?

##########
File path: helix-core/src/main/java/org/apache/helix/cloud/event/helix/CloudEventCallbackProperty.java
##########
@@ -0,0 +1,227 @@
+package org.apache.helix.cloud.event.helix;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import org.apache.helix.HelixManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A property for users to customize the behavior of a Helix manager as a cloud event listener
+ * Use this
+ */
+public class CloudEventCallbackProperty {
+  private static final Logger LOG =
+      LoggerFactory.getLogger(CloudEventCallbackProperty.class.getName());
+
+  // The key for user to pass in callback impl class name in the constructor
+  public static final String CALLBACK_IMPL_CLASS_NAME = "callbackImplClassName";

Review comment:
       Let's have a constant class to hold all the Helix defined entries.

##########
File path: helix-core/src/main/java/org/apache/helix/cloud/event/helix/CloudEventCallbackProperty.java
##########
@@ -0,0 +1,227 @@
+package org.apache.helix.cloud.event.helix;
+
+/*
+ * 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.
+ */
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
+
+import org.apache.helix.HelixManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A property for users to customize the behavior of a Helix manager as a cloud event listener
+ * Use this
+ */
+public class CloudEventCallbackProperty {
+  private static final Logger LOG =
+      LoggerFactory.getLogger(CloudEventCallbackProperty.class.getName());
+
+  // The key for user to pass in callback impl class name in the constructor
+  public static final String CALLBACK_IMPL_CLASS_NAME = "callbackImplClassName";
+  private Map<OnPauseOperations, BiConsumer<HelixManager, Object>>
+      _onPauseOperationMap;
+  private Map<OnResumeOperations, BiConsumer<HelixManager, Object>>
+      _onResumeOperationMap;
+  private final String _callbackImplClassName;
+
+  /**
+   * Constructor
+   * @param userArgs A map contains information that users pass in
+   */
+  public CloudEventCallbackProperty(Map<String, String> userArgs) {
+    _callbackImplClassName = userArgs.get(CALLBACK_IMPL_CLASS_NAME);
+    _onPauseOperationMap = new ConcurrentHashMap<>();
+    _onResumeOperationMap = new ConcurrentHashMap<>();
+  }
+
+  /**
+   * A collection of types of optional Helix-supported operations
+   */
+  public enum OptionalHelixOperation {
+    MAINTENANCE_MODE
+  }
+
+  /**
+   * A collection of types and positions for user to plugin customized callback
+   */
+  public enum UserDefinedCallbackType {
+    PRE_ON_PAUSE, POST_ON_PAUSE, PRE_ON_RESUME, POST_ON_RESUME,
+  }
+
+  private enum OnPauseOperations {
+    PRE_ON_PAUSE, MAINTENANCE_MODE, DEFAULT_HELIX_OPERATION, POST_ON_PAUSE
+  }
+
+  private enum OnResumeOperations {
+    PRE_ON_RESUME, DEFAULT_HELIX_OPERATION, MAINTENANCE_MODE, POST_ON_RESUME,
+  }
+
+  /**
+   * Trigger the registered onPause callbacks one by one
+   * Order:
+   * 1. PRE onPause
+   * 2. Helix optional: Maintenance Mode
+   * 3. Helix default
+   * 4. POST onPause
+   * @param helixManager Helix manager to perform operations
+   * @param eventInfo Information of the event provided by upsteam
+   * @param callbackImplClass Implementation class for Helix default and optional operations
+   */
+  public void onPause(HelixManager helixManager, Object eventInfo,
+      AbstractCloudEventCallbackImpl callbackImplClass) {
+    for (OnPauseOperations operation : OnPauseOperations.values()) {
+      switch (operation) {
+        case DEFAULT_HELIX_OPERATION:
+          callbackImplClass.onPauseDefaultHelixOperation(helixManager, eventInfo);
+          break;
+        case MAINTENANCE_MODE:
+          callbackImplClass.onPauseMaintenanceMode(helixManager, eventInfo);
+          break;
+        default:
+          _onPauseOperationMap.getOrDefault(operation, (manager, info) -> {
+          }).accept(helixManager, eventInfo);
+      }
+    }
+  }
+
+  /**
+   * Trigger the registered onResume callbacks one by one
+   * Order:
+   * 1. PRE onResume
+   * 2. Helix default
+   * 3. Helix optional: Maintenance Mode
+   * 4. POST onResume
+   * @param helixManager Helix manager to perform operations
+   * @param eventInfo Information of the event provided by upsteam
+   * @param callbackImplClass Implementation class for Helix default and optional operations
+   */
+  public void onResume(HelixManager helixManager, Object eventInfo,

Review comment:
       They are duplicated. Please consider use two property object. So you do not need to write the function again.

##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -679,6 +710,40 @@ public String getClusterName() {
     return _clusterName;
   }
 
+  /**
+   * CloudEventListener
+   */
+  @Override
+  public void onPause(Object eventInfo) {
+    _cloudEventCallbackProperty.onPause(this, eventInfo, _callbackImplClass);
+  }
+
+  @Override
+  public void onResume(Object eventInfo) {
+    _cloudEventCallbackProperty.onResume(this, eventInfo, _callbackImplClass);
+  }
+
+  @Override
+  public ListenerType getListenerType() {
+    return ListenerType.UNORDERED;
+  }
+
+  private AbstractCloudEventCallbackImpl loadCloudEventCallbackImplClass(String implClassName) {
+    AbstractCloudEventCallbackImpl implClass;
+    try {
+      LOG.info("Loading class: " + implClassName);
+      implClass = (AbstractCloudEventCallbackImpl) HelixUtil.loadClass(getClass(), implClassName)
+          .newInstance();
+    } catch (Exception e) {
+      String errMsg = String
+          .format("No cloud event callback implementation class found for: %s. message: ",
+              implClassName);
+      LOG.error(errMsg, e);
+      throw new HelixException(String.format(errMsg, e));

Review comment:
       I am thinking about would this be a critical thing. Let's say if the class is not load, can we log error but let others work? Then we can just use default abstract class as the usage.




-- 
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: reviews-unsubscribe@helix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org