[GitHub] [geode] kirklund commented on a change in pull request #6225: GEODE-9101: fix VisibleNode attribute in MemberMXBean

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

kirklund commented on a change in pull request #6225:
URL: https://github.com/apache/geode/pull/6225#discussion_r605267417



##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
##########
@@ -1881,9 +1881,8 @@ public void handleManagerDeparture(InternalDistributedMember theId, boolean memb
     removeHostedLocators(theId);
     redundancyZones.remove(theId);
 
-    if (theId.getVmKind() != ClusterDistributionManager.LOCATOR_DM_TYPE) {
-      stats.incNodes(-1);
-    }
+    stats.incNodes(-1);
+

Review comment:
       I think you're trying to change "nodes" to show the number of "servers" in the cluster but it's a different concept. "nodes" is a statistic in DistributionStats that reflects the number of peer-to-peer processes in the cluster, and we really need this statistic to remain as is.
   
   I recommend adding one or more new attribute(s) to MemberMXBean or DistributedSystemMXBean which reflect the number of "non-locator members" or "servers". There are several related operations on DistributedSystemMXBean such listLocators(), listMembers(), listServers(). All three of these are "nodes".
   
   listMembers() should show you all of the locators and servers (ie, any peer-to-peer process) in the cluster.
   
   listLocators() should should you all of the locators only.
   
   listServers() should show you all of the servers only.
   
   There is a subtle difference between Attributes and Operations in JMX. You probably want an Attribute in this case so that it can be monitored with JMX APIs. But I'll leave further planning to you.




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