resolve conflicts

Author: liuxiaocs7

hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionInTransitionTracker.java

@@ -17,6 +17,7 @@
  */
 package org.apache.hadoop.hbase.master.assignment;
 
+import com.google.errorprone.annotations.RestrictedApi;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
@@ -39,34 +40,30 @@
 @InterfaceAudience.Private
 public class RegionInTransitionTracker {
   private static final Logger LOG = LoggerFactory.getLogger(RegionInTransitionTracker.class);
-  private static final LongConsumer NOOP_RIT_DURATION_CONSUMER = ignored -> {
-  };
 
   private static final List<RegionState.State> DISABLE_TABLE_REGION_STATE =
     List.of(RegionState.State.OFFLINE, RegionState.State.CLOSED);
 
   private static final List<RegionState.State> ENABLE_TABLE_REGION_STATE =
     List.of(RegionState.State.OPEN);
 
-  // DO NOT USE containsKey()/remove() on regionInTransition with a different RegionInfo instance:
-  // this map is ordered by RegionInfo.COMPARATOR, and that comparator includes the offline flag.
-  // Lookups can therefore fail if the RegionInfo used as the key has a different offline value,
-  // even when it refers to the same region. Offline value changes with splitting.
+  // DO NOT USE containsKey()/remove() on regionInTransition with a RegionInfo instance whose
+  // offline flag differs from the one stored as the key: RegionInfo#equals and #hashCode both
+  // include the offline flag, so such a lookup misses even when it refers to the same region.
+  // Offline value changes with splitting.
   private final ConcurrentHashMap<RegionInfo, Pair<RegionStateNode, Long>> regionInTransition =
     new ConcurrentHashMap<>();
 
   private final LongConsumer ritDurationConsumer;
   private TableStateManager tableStateManager;
 
-  public RegionInTransitionTracker() {
-    this(NOOP_RIT_DURATION_CONSUMER);
-  }
-
   public RegionInTransitionTracker(LongConsumer ritDurationConsumer) {
     this.ritDurationConsumer = Objects.requireNonNull(ritDurationConsumer);
   }
 
-  public boolean isRegionInTransition(final RegionInfo regionInfo) {
+  @RestrictedApi(explanation = "Should only be called in tests", link = "",
+      allowedOnPath = ".*/src/test/.*")
+  boolean isRegionInTransition(final RegionInfo regionInfo) {
     return regionInTransition.containsKey(regionInfo);
   }
 
@@ -75,11 +72,10 @@ public boolean isRegionInTransition(final RegionInfo regionInfo) {
    * it was hosting are automatically added to the RIT list since they need to be reassigned to
    * other servers.
    * @param regionStateNode the region whose hosting server crashed
-   * @param crashTime       the time the hosting server crashed, used as the RIT start time. We take
-   *                        it explicitly instead of reading {@link RegionStateNode#getLastUpdate()}
-   *                        because the latter is masked by a stale procedure that may still be
-   *                        attached from before the server died, which would anchor the duration at
-   *                        the wrong time. A non-positive value falls back to the node's last
+   * @param crashTime       the RIT start time to use when the region is not already in transition
+   *                        (an existing entry keeps its earlier start). Passed explicitly rather
+   *                        than read from {@link RegionStateNode#getLastUpdate()}, which a stale
+   *                        procedure can mask. A non-positive value falls back to the node's last
    *                        update.
    */
   public void regionCrashed(RegionStateNode regionStateNode, long crashTime) {
@@ -171,7 +167,7 @@ private boolean removeRegionInTransition(final RegionInfo regionInfo) {
     Pair<RegionStateNode, Long> removed = regionInTransition.remove(regionInfo);
     if (removed != null) {
       long duration = EnvironmentEdgeManager.currentTime() - removed.getSecond();
-      if (duration > 0) {
+      if (duration >= 0) {
         ritDurationConsumer.accept(duration);
       }
     }
@@ -205,5 +201,4 @@ public void setTableStateManager(TableStateManager tableStateManager) {
   private static boolean isReplica(RegionStateNode regionStateNode) {
     return regionStateNode.getRegionInfo().getReplicaId() != RegionInfo.DEFAULT_REPLICA_ID;
   }
-
 }

hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManagerRitDurationMetrics.java

@@ -26,6 +26,7 @@
 import org.apache.hadoop.hbase.TableName;
 import org.apache.hadoop.hbase.client.RegionInfo;
 import org.apache.hadoop.hbase.client.Table;
+import org.apache.hadoop.hbase.master.assignment.AssignmentTestingUtil;
 import org.apache.hadoop.hbase.testclassification.MasterTests;
 import org.apache.hadoop.hbase.testclassification.MediumTests;
 import org.apache.hadoop.hbase.util.Bytes;
@@ -49,7 +50,6 @@ public class TestAssignmentManagerRitDurationMetrics {
 
   private static HMaster MASTER;
   private static final String RIT_DURATION_NUM_OPS_METRIC = "RitDuration_num_ops";
-  private static final String RIT_DURATION_MAX_METRIC = "RitDuration_max";
 
   @BeforeAll
   public static void startCluster() throws Exception {
@@ -69,7 +69,7 @@ public void testRitDurationHistogramMetric(TestInfo testInfo) throws Exception {
       RegionInfo regionInfo =
         MASTER.getAssignmentManager().getRegionStates().getRegionsOfTable(tableName).get(0);
       TEST_UTIL.waitFor(WAIT_TIMEOUT_MS,
-        () -> !MASTER.getAssignmentManager().isRegionInTransition(regionInfo)
+        () -> !AssignmentTestingUtil.isRegionInTransition(regionInfo, MASTER.getAssignmentManager())
           && MASTER.getAssignmentManager().getRegionStates().getRegionServerOfRegion(regionInfo)
               != null);
 
@@ -85,16 +85,17 @@ public void testRitDurationHistogramMetric(TestInfo testInfo) throws Exception {
         .orElseThrow(() -> new IllegalStateException("Need at least two regionservers"));
 
       TEST_UTIL.getAdmin().move(regionInfo.getEncodedNameAsBytes(), target);
-      TEST_UTIL.waitFor(WAIT_TIMEOUT_MS,
-        () -> target.equals(
-          MASTER.getAssignmentManager().getRegionStates().getRegionServerOfRegion(regionInfo))
-          && !MASTER.getAssignmentManager().isRegionInTransition(regionInfo));
+      TEST_UTIL.waitFor(WAIT_TIMEOUT_MS, () -> target
+        .equals(MASTER.getAssignmentManager().getRegionStates().getRegionServerOfRegion(regionInfo))
+        && !AssignmentTestingUtil.isRegionInTransition(regionInfo, MASTER.getAssignmentManager()));
 
-      MetricsRecord metricsRecord = snapshotMetrics(amSource);
-      assertEquals(ritDurationNumOps + 1,
-        getMetricValue(metricsRecord, RIT_DURATION_NUM_OPS_METRIC));
-      assertTrue(getMetricValue(metricsRecord, RIT_DURATION_MAX_METRIC) > 0,
-        "ritDuration histogram should export a positive max value");
+      // num_ops is cumulative (never reset on snapshot); an increase proves the histogram is now
+      // fed on RIT completion. Use >= not ==: background RIT may also add. _max is not asserted --
+      // snapshot() resets it on every read, racing the metrics2 sampler.
+      long ritDurationNumOpsAfter =
+        getMetricValue(snapshotMetrics(amSource), RIT_DURATION_NUM_OPS_METRIC);
+      assertTrue(ritDurationNumOpsAfter >= ritDurationNumOps + 1,
+        "RitDuration histogram num_ops should increase after a region transition");
     }
   }