mnpoonia commented on PR #8472:
URL: https://github.com/apache/hbase/pull/8472#issuecomment-5055293864
### ๐ 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
```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();
}
}
}
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>
---
๐ก๏ธ Security Considerations
No security concerns. This is a defensive operational feature.
---
๐ฏ Performance Implications
Concern: The findBrokenRegionChain() scan runs on every normalization pass
for every table.
Analysis:
- The loop is O(n) where n = number of regions per table
- Operations are lightweight (byte array comparisons)
- The check only runs if isSkipBrokenChain() is true (default)
- The warning log ensures operators know when the check fires
Verdict: Performance impact is negligible for normal tables. For tables
with thousands of regions, the scan is still fast (microseconds). The
early-exit when a problem is found is good.
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.
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
