[GitHub] [hbase] virajjasani commented on a change in pull request #2761: HBASE-25329 Dump region hashes in logs for the regions that are stuck in transition for more than a configured amount of time

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

virajjasani commented on a change in pull request #2761:
URL: https://github.com/apache/hbase/pull/2761#discussion_r551793777



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
##########
@@ -1447,6 +1453,12 @@ private void update(final Collection<RegionState> regions, final long currentTim
         if (oldestRITTime < ritTime) {
           oldestRITTime = ritTime;
         }
+        if (counter < 500) { // Record 500 oldest RITs

Review comment:
       Just in case cluster is already facing some perf issues, we might want to reduce this count. Let's make this configurable.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
##########
@@ -1455,6 +1467,10 @@ private void updateRegionsInTransitionMetrics(final RegionInTransitionStat ritSt
     metrics.updateRITOldestAge(ritStat.getOldestRITTime());
     metrics.updateRITCount(ritStat.getTotalRITs());
     metrics.updateRITCountOverThreshold(ritStat.getTotalRITsOverThreshold());
+    if ((EnvironmentEdgeManager.currentTime() - ritStat.ritThreshold / 2) >= ritStat.statTimestamp){
+      LOG.debug("Oldest RIT hashes and states: "+ritStat.getOldestRITHashesAndStates().toString());

Review comment:
       Since we are anyways exposing rits in metrics, writing DEBUG logs might be overkill I believe. Let's keep the log at TRACE level?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java
##########
@@ -68,6 +68,21 @@ public int compare(final RegionState l, final RegionState r) {
   public final static RegionStateStampComparator REGION_STATE_STAMP_COMPARATOR =
       new RegionStateStampComparator();
 
+  // This comparator sorts the RegionStates by duration then Region name.
+  // Comparing by duration alone can lead us to discard different RegionStates that happen
+  // to share a duration.
+  private static class RegionStateDurationComparator implements Comparator<RegionState> {
+    @Override
+    public int compare(RegionState l, RegionState r) {
+      return Long.compare(l.getRitDuration(), r.getRitDuration()) == 0 ?
+        Bytes.compareTo(l.getRegion().getRegionName(), r.getRegion().getRegionName()) :
+        Long.compare(l.getRitDuration(), r.getRitDuration());
+    }
+  }
+
+  public final static RegionStateDurationComparator REGION_STATE_DURATION_COMPARATOR =

Review comment:
       nit: keep it private?




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