[GitHub] [hbase] Apache9 commented on a change in pull request #3610: HBASE-26163 Better logging in RSGroupInfoManagerImpl

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

Apache9 commented on a change in pull request #3610:
URL: https://github.com/apache/hbase/pull/3610#discussion_r693349265



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupBasedLoadBalancer.java
##########
@@ -234,6 +237,9 @@ public ServerName randomAssignment(RegionInfo region,
       if (!fallbackRegions.isEmpty()) {
         List<ServerName> candidates = null;
         if (isFallbackEnabled()) {
+          LOG.debug("Falling back {} regions to servers outside their RSGroup. Regions: {}",

Review comment:
       Better wrap it with a LOG.isDebugEnabled? We have a stream operation when constructing the parameter.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java
##########
@@ -655,22 +661,27 @@ private synchronized void flushConfig(Map<String, RSGroupInfo> newGroupMap) thro
       // according to the inputted newGroupMap (an updated copy of rsGroupMap)
       this.holder = new RSGroupInfoHolder(newGroupMap);
 
+      LOG.info("New RSGroup map: {}", newGroupMap.toString());
+
       // Do not need to update tableMap
       // because only the update on servers in default group is allowed above,
       // or IOException will be thrown
       return;
     }
 
     /* For online mode, persist to hbase:rsgroup and Zookeeper */
+    LOG.debug("Online mode, persisting to {} and ZK", RSGROUP_TABLE_NAME);
     flushConfigTable(newGroupMap);
 
     // Make changes visible after having been persisted to the source of truth
     resetRSGroupMap(newGroupMap);
     saveRSGroupMapToZK(newGroupMap);
     updateCacheOfRSGroups(newGroupMap.keySet());
+    LOG.info("Flush config done, new RSGroup map: {}", newGroupMap.toString());

Review comment:
       Ditto.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java
##########
@@ -1205,6 +1220,8 @@ private void moveTablesAndWait(Set<TableName> tables, String targetGroup) throws
       ProcedureSyncWait.waitForProcedureToCompleteIOE(masterServices.getMasterProcedureExecutor(),
           proc, Long.MAX_VALUE);
     }
+    LOG.info("Move tables done. Moved {} tables to {}: {}", tables.size(), targetGroup,

Review comment:
       Ditto.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java
##########
@@ -655,22 +661,27 @@ private synchronized void flushConfig(Map<String, RSGroupInfo> newGroupMap) thro
       // according to the inputted newGroupMap (an updated copy of rsGroupMap)
       this.holder = new RSGroupInfoHolder(newGroupMap);
 
+      LOG.info("New RSGroup map: {}", newGroupMap.toString());

Review comment:
       Maybe too much for info?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupInfoManagerImpl.java
##########
@@ -256,7 +256,9 @@ private synchronized void updateDefaultServers() {
     newGroupMap.put(RSGroupInfo.DEFAULT_GROUP, newDefaultGroupInfo);
     // do not need to persist, as we do not persist default group.
     resetRSGroupMap(newGroupMap);
-    LOG.info("Updated default servers, {} servers", newDefaultGroupInfo.getServers().size());
+    LOG.info("Updated default servers, now {} servers online: {}",
+      newDefaultGroupInfo.getServers().size(),
+      newDefaultGroupInfo.getServers().stream().map(Address::toString).collect(Collectors.toSet()));

Review comment:
       I think the message will be too long if we log the servers? This is a log in info level, not debug.




-- 
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: issues-unsubscribe@hbase.apache.org

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