[GitHub] [geode] jchen21 commented on a change in pull request #5403: GEODE-8385: hang recovering from disk with cyclic dependencies

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

jchen21 commented on a change in pull request #5403:
URL: https://github.com/apache/geode/pull/5403#discussion_r461788593



##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1866,39 +1848,56 @@ public void handleConsoleShutdown(InternalDistributedMember theId, boolean crash
   void shutdownMessageReceived(InternalDistributedMember theId, String reason) {
     removeHostedLocators(theId);
     distribution.shutdownMessageReceived(theId, reason);
+    handleManagerDeparture(theId, false, reason);

Review comment:
       Why this is not `handleManagerDeparture(theId, false, reason, true)`? `handleManagerDeparture(theId, false, reason)` will not update the `stats` in line 1887.

##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1866,39 +1848,56 @@ public void handleConsoleShutdown(InternalDistributedMember theId, boolean crash
   void shutdownMessageReceived(InternalDistributedMember theId, String reason) {
     removeHostedLocators(theId);
     distribution.shutdownMessageReceived(theId, reason);
+    handleManagerDeparture(theId, false, reason);
   }
 
+  /*
+   * handleManagerDeparted may be invoked multiple times for a member identifier.
+   * We allow this and inform listeners on each invocation, but only perform some
+   * actions (such as decrementing the node count) if the change came from a
+   * membership view.
+   */
   @Override
-  public void handleManagerDeparture(InternalDistributedMember theId, boolean p_crashed,
-      String p_reason) {
+  public void handleManagerDeparture(InternalDistributedMember theId, boolean memberCrashed,
+      String reason) {
+    handleManagerDeparture(theId, memberCrashed, reason, false);
+  }
 
+  private void handleManagerDeparture(InternalDistributedMember theId, boolean memberCrashed,
+      String reason, boolean fromViewChange) {
     alertingService.removeAlertListener(theId);
 
+    removeUnfinishedStartup(theId, true);
+
     int vmType = theId.getVmKind();
     if (vmType == ADMIN_ONLY_DM_TYPE) {
-      removeUnfinishedStartup(theId, true);
-      handleConsoleShutdown(theId, p_crashed, p_reason);
+      handleConsoleShutdown(theId, memberCrashed, reason);
       return;
     }
 
-    removeUnfinishedStartup(theId, true);
-
-    if (removeManager(theId, p_crashed, p_reason)) {
-      if (theId.getVmKind() != ClusterDistributionManager.LOCATOR_DM_TYPE) {
-        stats.incNodes(-1);
-      }
-      String msg;
-      if (p_crashed && !shouldInhibitMembershipWarnings()) {
-        msg =
-            "Member at {} unexpectedly left the distributed cache: {}";
-        addMemberEvent(new MemberCrashedEvent(theId, p_reason));
-      } else {
-        msg =
-            "Member at {} gracefully left the distributed cache: {}";
-        addMemberEvent(new MemberDepartedEvent(theId, p_reason));
-      }
-      logger.info(msg, new Object[] {theId, prettifyReason(p_reason)});
+    if (logger.isDebugEnabled()) {
+      logger.debug(
+          "DistributionManager: removing member <{}>; crashed {}; reason = {} fromView = {}", theId,
+          memberCrashed, prettifyReason(reason), fromViewChange);
+    }
+    removeHostedLocators(theId);
+    redundancyZones.remove(theId);
 
+    if (fromViewChange && theId.getVmKind() != ClusterDistributionManager.LOCATOR_DM_TYPE) {
+      stats.incNodes(-1);
+    }
+    String msg;
+    if (memberCrashed && !shouldInhibitMembershipWarnings()) {
+      msg =
+          "Member at {} unexpectedly left the distributed cache: {}";
+      addMemberEvent(new MemberCrashedEvent(theId, reason));
+    } else {
+      msg =
+          "Member at {} gracefully left the distributed cache: {}";
+      addMemberEvent(new MemberDepartedEvent(theId, reason));
+    }
+    if (fromViewChange) {
+      logger.info(msg, new Object[] {theId, prettifyReason(reason)});

Review comment:
       Would it better to move the `logger.info` out of the `if` statement? So regardless the value of `fromViewChange`, it logs the member departure. It will be helpful when analyzing the logs. 

##########
File path: geode-membership/src/main/java/org/apache/geode/distributed/internal/membership/gms/GMSMembership.java
##########
@@ -688,7 +688,11 @@ private void removeWithViewLock(ID dm, boolean crashed, String reason) {
       return; // Explicit deletion, no upcall.
     }
 
-    listener.memberDeparted(dm, crashed, reason);
+    if (!shutdownMembers.containsKey(dm)) {

Review comment:
       👍 




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