[GitHub] [helix] junkaixue commented on a change in pull request #1771: Add model to record history and status of management mode

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

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



##########
File path: helix-core/src/main/java/org/apache/helix/model/ControllerHistory.java
##########
@@ -136,6 +146,52 @@ public ZNRecord updateHistory(String clusterName, String instanceName, String ve
     return historyList;
   }
 
+  /**
+   * Gets the management mode history.
+   *
+   * @return List of history json strings.
+   */
+  public List<String> getManagementModeHistory() {
+    List<String> history =
+        _record.getListField(ManagementModeConfigKey.MANAGEMENT_MODE_HISTORY.name());
+    return history == null ? Collections.emptyList() : history;
+  }
+
+  /**
+   * Updates management mode and status history to controller history in FIFO order.
+   *
+   * @param controller controller name
+   * @param mode cluster management mode {@link ClusterManagementMode}
+   * @param fromHost the hostname that creates the management mode signal
+   * @param time time in millis
+   * @param reason reason to put the cluster in management mode
+   * @return updated history znrecord
+   */
+  public ZNRecord updateManagementModeHistory(String controller, ClusterManagementMode mode,

Review comment:
       We have a history update function for instance and controller. Can we resue the code or let's put it into some comment place to avoid redundant code?

##########
File path: helix-core/src/main/java/org/apache/helix/PropertyKey.java
##########
@@ -241,6 +243,13 @@ public PropertyKey clusterConfig() {
           _clusterName, ConfigScopeProperty.CLUSTER.toString(), _clusterName);
     }
 
+    /**
+     * Get a property key associated with this cluster status
+     * @return {@link PropertyKey}
+     */
+    public PropertyKey clusterStatus() {
+      return new PropertyKey(STATUS, ClusterStatus.class, _clusterName);
+    }

Review comment:
       Question: from this property key, it looks like on the top of root path? 

##########
File path: helix-core/src/main/java/org/apache/helix/model/ControllerHistory.java
##########
@@ -136,6 +146,52 @@ public ZNRecord updateHistory(String clusterName, String instanceName, String ve
     return historyList;
   }
 
+  /**
+   * Gets the management mode history.
+   *
+   * @return List of history json strings.
+   */
+  public List<String> getManagementModeHistory() {
+    List<String> history =
+        _record.getListField(ManagementModeConfigKey.MANAGEMENT_MODE_HISTORY.name());
+    return history == null ? Collections.emptyList() : history;
+  }
+
+  /**
+   * Updates management mode and status history to controller history in FIFO order.
+   *
+   * @param controller controller name
+   * @param mode cluster management mode {@link ClusterManagementMode}
+   * @param fromHost the hostname that creates the management mode signal
+   * @param time time in millis
+   * @param reason reason to put the cluster in management mode
+   * @return updated history znrecord
+   */
+  public ZNRecord updateManagementModeHistory(String controller, ClusterManagementMode mode,
+      String fromHost, long time, String reason) {
+    List<String> historyList = getManagementModeHistory();
+    if (historyList.isEmpty()) {
+      historyList = new ArrayList<>();
+      _record.setListField(ManagementModeConfigKey.MANAGEMENT_MODE_HISTORY.name(), historyList);
+    }
+
+    if (historyList.size() >= HISTORY_SIZE) {
+      historyList = historyList.subList(historyList.size() - HISTORY_SIZE + 1, historyList.size());
+    }
+
+    Map<String, String> historyEntry = new HashMap<>();
+    historyEntry.put(ConfigProperty.CONTROLLER.name(), controller);
+    historyEntry.put(ConfigProperty.TIME.name(), Instant.ofEpochMilli(time).toString());
+    historyEntry.put(ManagementModeConfigKey.MODE.name(), mode.getMode().name());
+    historyEntry.put(ManagementModeConfigKey.STATUS.name(), mode.getStatus().name());
+    historyEntry.put(PauseSignal.PauseSignalProperty.FROM_HOST.name(), fromHost);

Review comment:
       Is this from host necessary? We can make it optional.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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



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