[GitHub] [geode] sabbey37 commented on a change in pull request #6567: GEODE-9070: Improve ClientServerSessionCacheDUnitTest and add additional logging

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

sabbey37 commented on a change in pull request #6567:
URL: https://github.com/apache/geode/pull/6567#discussion_r646685684



##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1082,7 +1082,16 @@ public InternalDistributedMember getId() {
       if (observer != null) {
         observer.beforeSendMessage(this, msg);
       }
-      return sendMessage(msg);
+      Set<InternalDistributedMember> membersNotSent = sendMessage(msg);
+
+      if (membersNotSent != null && !membersNotSent.isEmpty()) {
+        logger.warn(
+            "The following members: {} were not properly sent the message: {}. If unhandled this may"
+                + " cause the result processor to hang while collecting results.",
+            membersNotSent, msg.getShortClassName());
+      }
+
+      return membersNotSent;

Review comment:
       Thanks for the observations, Bill! Right before the `"Failed to send message..."` log in `DistributionImpl.directChannelSend()`, we perform the following check:
   ```
    if (!membership.hasMember(member) || (th instanceof ShunnedMemberException)) {
             continue;
     }
   ```
   which effective skips the log for those cases.  The `ShunnedMemberException` is the one we encountered here.  We could log before checking those cases or log something different within that if block and remove the log we added to `ClusterDistributionManager`.  What do you think is appropriate?
   
   Regarding the "result processor"-specific message, that is a good question.  The "result processor" in this context is `ReplyProcessor21` (so perhaps that should say `reply processor` instead).  I think that's a good call out though, we probably shouldn't assume that if a message is sent there is always a reply processor involved.  It seems like we should remove this log anyway in favor of logging in `DistributionImpl` instead, so maybe we could just stick with the `"Failed to send message..." log there.




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