HBASE-29710 Skip normalization when tableRegions contains overlaps/holes

[SwaraliJoshi]

Splitting/merging a table whose region chain already has holes or overlaps can increase the number of holes/overlaps and complicate the eventual repair by the CatalogJanitor/HBCK. The merge path is protected downstream by MergeTableRegionsProcedure (force=false rejects non-adjacent/non-overlapping regions), but the split path had no such guard.

Add a scheduler-level check in SimpleRegionNormalizer that scans the sorted region chain for a hole, overlap, or degenerate region and skips normalization (both split and merge) for that table when the chain is broken. Guarded by a new config toggle hbase.normalizer.skip.broken.chain (default true, documented in hbase-default.xml) and emits a WARN log naming the offending region pair. The merge-side validation is left untouched.

[mnpoonia]

🔍 Issues & Suggestions

1. Code Duplication: getRegionInfoWithLargestEndKey()

Issue: The new getRegionInfoWithLargestEndKey() method in SimpleRegionNormalizer is nearly identical to MetaFixer.getRegionInfoWithLargestEndKey(). This violates DRY and creates a maintenance burden.

Location: SimpleRegionNormalizer.java:344-361

Recommendation:

• Move MetaFixer.getRegionInfoWithLargestEndKey() to a shared utility class (e.g., RegionChainValidator or add it to RegionInfo as a static helper)
• Both SimpleRegionNormalizer and MetaFixer should reference the shared implementation
• This ensures the logic stays in sync and benefits from any future improvements

// Suggested location: org.apache.hadoop.hbase.util.RegionChainValidator
public static RegionInfo getRegionInfoWithLargestEndKey(RegionInfo a, RegionInfo b) {
  // shared implementation
}

---
2. Incomplete Overlap Detection Logic

Issue: The overlap detection in findBrokenRegionChain() has redundant/overlapping checks that could miss edge cases or confuse maintainers.

Location: SimpleRegionNormalizer.java:316-329

The current logic checks:
if (!previous.isNext(current)) {
  if (previous.isOverlap(current)) {
    // overlap between previous and current
  } else if (current.isOverlap(highestEndKeyRegionInfo)) {
    // overlap between current and highest-seen
  } else if (!highestEndKeyRegionInfo.isNext(current)) {
    // hole between highest-seen and current
  }
} else if (current.isOverlap(highestEndKeyRegionInfo)) {
  // Adjacent to previous, but overlaps an earlier region
}

Problems:
- The outer if (!previous.isNext(current)) branch already handles the overlap case with previous.isOverlap(current), but then also checks current.isOverlap(highestEndKeyRegionInfo) — this could be redundant if previous ==
highestEndKeyRegionInfo
- The else if at the end (line 327-330) seems to handle a corner case where regions are adjacent but still overlap a wider region — but this should already be caught by the second check in the first branch

Recommendation:
Simplify the logic to match CatalogJanitor's ReportMakingVisitor pattern more closely:

if (previous != null) {
  // Check for hole (gap between highest-seen and current)
  if (!highestEndKeyRegionInfo.isNext(current)) {
    if (highestEndKeyRegionInfo.isOverlap(current) || current.isOverlap(highestEndKeyRegionInfo)) {
      return "overlap between " + highestEndKeyRegionInfo.getShortNameToLog() + " and "
        + current.getShortNameToLog();
    } else {
      return "hole between " + highestEndKeyRegionInfo.getShortNameToLog() + " and "
        + current.getShortNameToLog();
    }
  }
}

This is clearer: once we have a highestEndKeyRegionInfo, we only need to check if it's adjacent to current. If not, it's either an overlap or a hole.

---
3. Degenerate Region Check Placement

Issue: The degenerate check happens inside the loop, but a degenerate region is a standalone problem that doesn't require comparing with previous.

Location: SimpleRegionNormalizer.java:310-312

Recommendation:
Move the degenerate check before the if (previous != null) block for clarity:

for (final RegionInfo current : tableRegions) {
  if (current.getReplicaId() != RegionInfo.DEFAULT_REPLICA_ID || current.isSplitParent()) {
    continue;
  }
  if (current.isDegenerate()) {
    return "degenerate region " + current.getShortNameToLog() + " (startKey > endKey)";
  }
  if (previous != null) {
    // ... adjacency/overlap/hole checks
  }
  previous = current;
  highestEndKeyRegionInfo = getRegionInfoWithLargestEndKey(highestEndKeyRegionInfo, current);
}

This makes the logic easier to follow: first validate the current region itself, then compare it with the chain.

---
4. Missing Edge Case: First Region Not Starting at Empty Key

Issue: The PR description mentions "Boundary (first/last) completeness is intentionally not checked here," but a table with a hole at the beginning (first region doesn't start at EMPTY_START_ROW) is a serious chain break.

Example:
Table regions: [b, d), [d, f), [f, )
This table has a hole before 'b', but findBrokenRegionChain() would return null.

Recommendation:
Either:
**Recommendation:**
- Move `MetaFixer.getRegionInfoWithLargestEndKey()` to a shared utility class (e.g., `RegionChainValidator` or add it to `RegionInfo` as a static helper)
- Both `SimpleRegionNormalizer` and `MetaFixer` should reference the shared implementation
- This ensures the logic stays in sync and benefits from any future improvements

```java
// Suggested location: org.apache.hadoop.hbase.util.RegionChainValidator
public static RegionInfo getRegionInfoWithLargestEndKey(RegionInfo a, RegionInfo b) {
  // shared implementation
}

---
2. Incomplete Overlap Detection Logic

Issue: The overlap detection in findBrokenRegionChain() has redundant/overlapping checks that could miss edge cases or confuse maintainers.

Location: SimpleRegionNormalizer.java:316-329

The current logic checks:
if (!previous.isNext(current)) {
  if (previous.isOverlap(current)) {
    // overlap between previous and current
  } else if (current.isOverlap(highestEndKeyRegionInfo)) {
    // overlap between current and highest-seen
  } else if (!highestEndKeyRegionInfo.isNext(current)) {
    // hole between highest-seen and current
  }
} else if (current.isOverlap(highestEndKeyRegionInfo)) {
  // Adjacent to previous, but overlaps an earlier region
}

Problems:
- The outer if (!previous.isNext(current)) branch already handles the overlap case with previous.isOverlap(current), but then also checks current.isOverlap(highestEndKeyRegionInfo) — this could be redundant if previous ==
highestEndKeyRegionInfo
- The else if at the end (line 327-330) seems to handle a corner case where regions are adjacent but still overlap a wider region — but this should already be caught by the second check in the first branch

Recommendation:
Simplify the logic to match CatalogJanitor's ReportMakingVisitor pattern more closely:

if (previous != null) {
  // Check for hole (gap between highest-seen and current)
  if (!highestEndKeyRegionInfo.isNext(current)) {
    if (highestEndKeyRegionInfo.isOverlap(current) || current.isOverlap(highestEndKeyRegionInfo)) {
      return "overlap between " + highestEndKeyRegionInfo.getShortNameToLog() + " and "
        + current.getShortNameToLog();
    } else {
      return "hole between " + highestEndKeyRegionInfo.getShortNameToLog() + " and "
        + current.getShortNameToLog();
    }
  }
}

7. Configuration Documentation

Issue: The hbase-default.xml description is clear, but doesn't mention the transient nature of the check (based on in-memory RegionStates, not hbase:meta).

Location: hbase-default.xml:668-673

Recommendation:
<description>Whether the normalizer should skip a table whose region chain is broken
  (contains a hole, an overlap, or a degenerate region). Normalizing (in particular
  splitting) such a table can increase the number of holes/overlaps and complicate the
  eventual repair by the CatalogJanitor/HBCK. The check is based on the in-memory
  RegionStates, which may briefly lag hbase:meta during transitions. Set to false to
  normalize regardless.</description>


Issue: The PR description mentions "Boundary (first/last) completeness is intentionally not checked here," but a table with a hole at the beginning (first region doesn't start at EMPTY_START_ROW) is a serious chain break.

Example:
Table regions: [b, d), [d, f), [f, )
This table has a hole before 'b', but findBrokenRegionChain() would return null.

Recommendation:
Either:
1. Document why this is intentional with a code comment explaining that this check is deliberately omitted because the normalizer doesn't care about boundary completeness
2. Add the check if boundary completeness should prevent normalization

If you choose to add the check:
// After filtering to default replicas, check first region
RegionInfo first = null;
for (RegionInfo ri : tableRegions) {
  if (ri.getReplicaId() == RegionInfo.DEFAULT_REPLICA_ID && !ri.isSplitParent()) {
    first = ri;
    break;
  }
}
if (first != null && !Bytes.equals(first.getStartKey(), HConstants.EMPTY_START_ROW)) {
  return "first region " + first.getShortNameToLog() + " does not start at beginning of table";
}

---
5. Test Coverage Gap: Non-Adjacent Overlaps

Issue: The existing testNoNormalizationWhenChainHasOverlap() test only covers a simple overlap case. The complex logic in findBrokenRegionChain() (lines 321-330) suggests it's trying to handle non-adjacent overlaps (e.g., region A=[,d),
region B=[b,f), region C=[f,)) where B is adjacent to C but overlaps A.

Recommendation:
Add a test case for this specific scenario:

@Test
public void testNoNormalizationWhenNonAdjacentOverlap() {
  final TableName tableName = this.tableName;
  // Region chain: [, b), [b, f), [d, )
  // The third region is "adjacent" to nothing, but overlaps the second
  final List<RegionInfo> regionInfos = new ArrayList<>();
  regionInfos.add(createRegionInfo(tableName, HConstants.EMPTY_START_ROW, Bytes.toBytes("b")));
  regionInfos.add(createRegionInfo(tableName, Bytes.toBytes("b"), Bytes.toBytes("f")));
  regionInfos.add(createRegionInfo(tableName, Bytes.toBytes("d"), HConstants.EMPTY_END_ROW));
  final Map<byte[], Integer> regionSizes = createRegionSizesMap(regionInfos, 5, 5, 30);
  setupMocksForNormalizer(regionSizes, regionInfos);

  // Should detect overlap between regions 2 and 3
  assertThat(normalizer.computePlansForTable(tableDescriptor), empty());
}

---
6. Warning Log Clarity

Issue: The warning message is good, but could include region count to help operators understand scale.

Location: SimpleRegionNormalizer.java:251-256

Recommendation:
LOG.warn("Skipping normalization of table {} ({} regions) because its region chain appears to be broken: {}. "
  + "If this persists, run the CatalogJanitor/HBCK to fix holes and overlaps. Set {}=false to normalize regardless.",
  table, ctx.getTableRegions().size(), brokenChainReason, SKIP_BROKEN_CHAIN_KEY);

---
7. Configuration Documentation

Issue: The hbase-default.xml description is clear, but doesn't mention the transient nature of the check (based on in-memory RegionStates, not hbase:meta).

Location: hbase-default.xml:668-673

Recommendation:
<description>Whether the normalizer should skip a table whose region chain is broken
  (contains a hole, an overlap, or a degenerate region). Normalizing (in particular
  splitting) such a table can increase the number of holes/overlaps and complicate the
  eventual repair by the CatalogJanitor/HBCK. The check is based on the in-memory
  RegionStates, which may briefly lag hbase:meta during transitions. Set to false to
  normalize regardless.</description>

 ---
🧪 Test Coverage

Good:
- ✅ Hole detection (testNoNormalizationWhenChainHasHole)
- ✅ Overlap detection (testNoNormalizationWhenChainHasOverlap)
- ✅ Config toggle (testBrokenChainGuardCanBeDisabled)
- ✅ Configuration observer (TestRegionNormalizerManagerConfigurationObserver.test())

Missing:
- ❌ Degenerate region detection (easy to add)
- ❌ Non-adjacent overlaps (the complex case from lines 327-330)
- ❌ Replicas and split-parent filtering (verify they're correctly skipped)

Recommendation: Add test for degenerate regions:

@Test
public void testNoNormalizationForDegenerateRegion() {
  final TableName tableName = this.tableName;
  final List<RegionInfo> regionInfos = new ArrayList<>();
  regionInfos.add(createRegionInfo(tableName, HConstants.EMPTY_START_ROW, Bytes.toBytes("b")));
  regionInfos.add(createRegionInfo(tableName, Bytes.toBytes("d"), Bytes.toBytes("c"))); // degenerate: d > c
  regionInfos.add(createRegionInfo(tableName, Bytes.toBytes("c"), HConstants.EMPTY_END_ROW));
  final Map<byte[], Integer> regionSizes = createRegionSizesMap(regionInfos, 10, 10, 30);
  setupMocksForNormalizer(regionSizes, regionInfos);

  assertThat(normalizer.computePlansForTable(tableDescriptor), empty());
}

---
📋 Summary & Recommendation

Verdict: ✅ Approve with minor changes

This is a solid, well-motivated defensive feature. The core logic is sound and borrows a proven pattern from CatalogJanitor. However, there are opportunities for improvement:

Required Changes:
1. Refactor getRegionInfoWithLargestEndKey() to a shared utility to eliminate duplication with MetaFixer
2. Simplify the overlap/hole detection logic to match CatalogJanitor's pattern more closely
3. Add test for degenerate regions to validate that branch

Recommended Changes:
4. Document or add the first-region boundary check
5. Add test for non-adjacent overlaps
6. Enhance configuration documentation with transience note
7. Add region count to warning log

Style/Convention Notes:
- Follows HBase conventions (logging, config keys, test structure)
- Javadoc is thorough
- Variable naming is clear

Great work overall! This will prevent a class of operational issues in production.