[GitHub] [helix] huizhilu commented on a change in pull request #1812: Implement Participant Freeze Process

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

huizhilu commented on a change in pull request #1812:
URL: https://github.com/apache/helix/pull/1812#discussion_r665852334



##########
File path: helix-core/src/main/java/org/apache/helix/participant/HelixStateMachineEngine.java
##########
@@ -146,18 +147,29 @@ private void sendNopMessage() {
 
   @Override
   public void reset() {
-    logger.info("Resetting HelixStateMachineEngine");
+    loopStateModelFactories(stateModel -> {
+      stateModel.reset();
+      String initialState = _stateModelParser.getInitialState(stateModel.getClass());
+      stateModel.updateState(initialState);
+    }, "reset");

Review comment:
       There are only 2 method names here. Enum seems too heavy as it would need to parse the real name: `RESET("reset"), SYNC_STATE("syncState")`. 

##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java
##########
@@ -1376,12 +1384,27 @@ public void handleNewSession(String sessionId) throws Exception {
     }
   }
 
-  void handleNewSessionAsParticipant(final String sessionId) throws Exception {
+  private void handleNewSessionInManagementMode(String sessionId) throws Exception {
+    LOG.info("Skip reset because instance is in {} status", LiveInstance.LiveInstanceStatus.PAUSED);
+    if (!InstanceType.PARTICIPANT.equals(_instanceType)) {

Review comment:
       I didn't add logic for CONTROLLER_PARTICIPANT. The logic is the same for CONTROLLER_PARTICIPANT. 
   `handleNewSessionAsParticipant(sessionId);` -> `handleNewSessionAsParticipant(sessionId, _liveInstanceInfoProvider);` I changed it to avoid below unnecessary api.
   ```
   handleNewSessionAsParticipant(sessionId) {
       handleNewSessionAsParticipant(sessionId, _liveInstanceInfoProvider);
   }
   ```

##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##########
@@ -413,13 +433,16 @@ private void carryOverPreviousCurrentState() {
      * remove previous current state parent nodes
      */
     for (String session : sessions) {
-      if (session.equals(_sessionId)) {
+      if (session.equals(sessionId)) {
         continue;
       }
 
-      String path = _keyBuilder.currentStates(_instanceName, session).getPath();
-      LOG.info("Removing current states from previous sessions. path: " + path);
-      _zkclient.deleteRecursively(path);
+      PropertyKey currentStatesProperty = keyBuilder.currentStates(instanceName, session);
+      String path = currentStatesProperty.getPath();
+      LOG.info("Removing current states from previous sessions. path: {}", path);
+      if (!dataAccessor.removeProperty(currentStatesProperty)) {

Review comment:
       Yes. Double checked. It will use recursively delete.

##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##########
@@ -162,6 +166,20 @@ public void handleNewSession() throws Exception {
     setupMsgHandler();
   }
 
+  private boolean shouldCarryOver() {
+    if (_liveInstanceInfoProvider == null) {
+      return true;
+    }
+    ZNRecord additionalLiveInstanceInfo = _liveInstanceInfoProvider.getAdditionalLiveInstanceInfo();

Review comment:
       Because each time `getAdditionalLiveInstanceInfo()` creates a ZNRecord object. We do it like that, we'll need to call `getAdditionalLiveInstanceInfo` twice, hence 2 ZNRecord objects.

##########
File path: helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java
##########
@@ -1305,6 +1330,43 @@ String getMessageTarget(String resourceName, String partitionName) {
     return String.format("%s_%s", resourceName, partitionName);
   }
 
+  private void changeParticipantStatus(String instanceName,
+      LiveInstance.LiveInstanceStatus toStatus, HelixManager manager) {
+    HelixDataAccessor accessor = manager.getHelixDataAccessor();
+    String sessionId = manager.getSessionId();
+    String path = accessor.keyBuilder().liveInstance(instanceName).getPath();
+
+    if (LiveInstance.LiveInstanceStatus.PAUSED.equals(toStatus)) {

Review comment:
       I also thought about it. It could be cleaner since there are too many if..else here. I didn't use it because I thought there are only 2 branches and we need to do null check for `toStatus`. Let me use it for code readers :) 




-- 
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