HBASE-30353 Prevent split parent regions from being re-opened by tanyastickles · Pull Request #8700 · apache/hbase

tanyastickles · 2026-09-24T17:39:08Z

Issue: https://issues.apache.org/jira/browse/HBASE-30353

We ran into an issue where a split parent region came back online.

What happened was:

The region split successfully.
There was an HMaster failover.
When the new HMaster was constructing its in memory data from the meta, it sees state=CLOSED, split=true, offline=true. (The in-memory state of the previous HMaster would have had state=SPLIT)
This ends up looking like an ordinary region stuck in closing rather than a split region.

…aster failover Split parents are permanently retired regions and must never be re-assigned. Two-part fix: 1. RegionStateStore.splitRegion() now writes SPLIT into info:state for the parent so that after a master failover, loadMeta reconstructs the parent with state=SPLIT rather than falling back to OFFLINE (the null-state path), which would make it eligible for re-assignment via processOfflineRegions(). 2. AssignmentManager guards isSplit() in both preTransitCheck (normal assign path) and createAssignProcedure (HBCK2/override/force path). The guard in createAssignProcedure sits before the override branch so that even force=true cannot bypass it. Together these cover the live-state case (state=SPLIT) and the post-failover case where an older meta row carries no info:state column and regionInfo is reconstructed with state=CLOSED but isSplit()=true. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

tanyastickles · 2026-09-24T18:25:15Z

    if (!regionNode.isInState(expectedStates)) {
      throw new DoNotRetryRegionException(UNEXPECTED_STATE_REGION + regionNode);
    }
+    // if we don't write the state as SPLIT to the meta, we would need this check.


comments are for context and discussion. i'll clean this up once we decide on an approach (persist SPLIT to state or just check the isSplit flag.)

Ok, after reviewing the state machine diagram on the official doc, it seems the SPLIT state should be a terminal state where a split region ends, so having a split region in CLOSED state seems inconsistent with the diagram. Therefore, setting the SPLIT state would be more compliant with the documentation.

Apache9

I think we have the same problem for mergeRegions method? We should also record the MERGED state for the merge parent regions I suppose.

Apache9 · 2026-09-28T03:08:16Z

    RegionStateNode regionNode = regionStates.getOrCreateRegionStateNode(regionInfo);
    regionNode.lock();
    try {
+      if (regionNode.isSplit()) {


So this is just for safety? We should not arrive here otherwise there should be bugs?

we still need this in case this method is called with override=true. it skips the preTransitCheck that validates that the region is CLOSED or OFFLINE.

We will only skip preTransitCheck if override and force are both true? I think this is only used for HBCK2 to fix broken cluster?

yep! the only way to end up in this path is when we're using HBCK2.

it looks like the only other place where we call createAssignProcedure with override=true and force=true is when we truncate a region (TruncateRegionProcedure.createAssignProcedure), but that's not a code path that's relevant for a split parent.

Apache9 · 2026-09-28T06:14:13Z

And please fix the spotless issues.

tanyastickles · 2026-09-28T18:14:59Z

I think we have the same problem for mergeRegions method? We should also record the MERGED state for the merge parent regions I suppose.

for MERGED, we should be safe.

when merging, we write a tombstone to the regionInfo (code), so if the hmaster rolls and we need to construct the state from meta, the region gets ignored.

we also set the state to CLOSED to explicitly avoid this issue. code

 // Set initial state to CLOSED.
    // NOTE: If initial state is not set to CLOSED then merged region gets added with the
    // default OFFLINE state. If Master gets restarted after this step, start up sequence of
    // master tries to assign this offline region. This is followed by re-assignments of the
    // merged region from resumed {@link MergeTableRegionsProcedure}
    MetaTableAccessor.addRegionStateToPut(putOfMerged, RegionInfo.DEFAULT_REPLICA_ID,
      RegionState.State.CLOSED);

in the split case, the meta state was CLOSED but because the regionInfo had the offline=true flag set, it still got assigned as an offline region. that won't happen in the MERGED case.

we could persist the state as MERGED to the meta for consistency in "terminal states are saved to the meta" but as far as i can tell, it shouldn't change the existing logic. i don't have strong opinions either way, so defer to your preference here!

Apache9 · 2026-09-29T02:50:33Z

I think we have the same problem for mergeRegions method? We should also record the MERGED state for the merge parent regions I suppose.

for MERGED, we should be safe.

when merging, we write a tombstone to the regionInfo (code), so if the hmaster rolls and we need to construct the state from meta, the region gets ignored.

we also set the state to CLOSED to explicitly avoid this issue. code
 // Set initial state to CLOSED.
    // NOTE: If initial state is not set to CLOSED then merged region gets added with the
    // default OFFLINE state. If Master gets restarted after this step, start up sequence of
    // master tries to assign this offline region. This is followed by re-assignments of the
    // merged region from resumed {@link MergeTableRegionsProcedure}
    MetaTableAccessor.addRegionStateToPut(putOfMerged, RegionInfo.DEFAULT_REPLICA_ID,
      RegionState.State.CLOSED);
in the split case, the meta state was CLOSED but because the regionInfo had the offline=true flag set, it still got assigned as an offline region. that won't happen in the MERGED case.

we could persist the state as MERGED to the meta for consistency in "terminal states are saved to the meta" but as far as i can tell, it shouldn't change the existing logic. i don't have strong opinions either way, so defer to your preference here!

The code you list in mergeRegions is for setting state to CLOSED for the merged child region, not the parents. In split we did the same thing for the split children.
But for merging, we just delete the parent regions from meta, but for split the region is still there, this is the difference I believe.

tanyastickles · 2026-09-29T18:10:29Z

I think we have the same problem for mergeRegions method? We should also record the MERGED state for the merge parent regions I suppose.

for MERGED, we should be safe.
when merging, we write a tombstone to the regionInfo (code), so if the hmaster rolls and we need to construct the state from meta, the region gets ignored.
we also set the state to CLOSED to explicitly avoid this issue. code
 // Set initial state to CLOSED.
    // NOTE: If initial state is not set to CLOSED then merged region gets added with the
    // default OFFLINE state. If Master gets restarted after this step, start up sequence of
    // master tries to assign this offline region. This is followed by re-assignments of the
    // merged region from resumed {@link MergeTableRegionsProcedure}
    MetaTableAccessor.addRegionStateToPut(putOfMerged, RegionInfo.DEFAULT_REPLICA_ID,
      RegionState.State.CLOSED);
in the split case, the meta state was CLOSED but because the regionInfo had the offline=true flag set, it still got assigned as an offline region. that won't happen in the MERGED case.
we could persist the state as MERGED to the meta for consistency in "terminal states are saved to the meta" but as far as i can tell, it shouldn't change the existing logic. i don't have strong opinions either way, so defer to your preference here!
The code you list in mergeRegions is for setting state to CLOSED for the merged child region, not the parents. In split we did the same thing for the split children. But for merging, we just delete the parent regions from meta, but for split the region is still there, this is the difference I believe.

oh shoot! you're totally right. my bad -- yeah it looks like the parent region are deleted from the meta.

in that case, are we good to proceed with the scope of the PR as is? or are there additional changes you'd recommend?

tanyastickles · 2026-09-29T20:35:53Z

we actually had a scenario test that covered the critsit case that we faced, so i'll also pull that in

…t isSplit check - Add testSplitParentCannotBeAssignedWhenStateIsOfflineAfterFailover to TestAssignmentManager, covering the exact pre-fix failover scenario (regionInfo.isSplit()=true but state=OFFLINE because the old code path did not write SPLIT to info:state). - Refactor the three split-parent assign tests to share a helper assertSplitParentAssignThrows(tableName, state). - Add testSplitRegionWritesSplitStateForParentInMeta to TestRegionStateStore, which calls splitRegion() against a real mini-cluster and asserts SPLIT is persisted to info:state — the core regression test for the fix. - Remove the redundant isSplit() guard with stale uncertain comments from preTransitCheck; the equivalent check in createAssignProcedure covers it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

TestSplitParentAssignment covers two behaviors introduced by the fix: - splitRegion writes State.SPLIT to info:state for the parent in meta - AssignmentManager.createAssignProcedure rejects split parents with DoNotRetryRegionException, preventing re-assignment after failover Ported from HubSpot#269, converted to JUnit 5 and updated MetaTableAccessor.getRegionStateColumn -> CatalogFamilyFormat.getRegionStateColumn to match the master branch module layout. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

wchevreuil

LGTM, +1.

Apache9 · 2026-10-06T13:25:32Z

+      "Precondition: state must be SPLIT (written to meta since HBASE-30353)");
+
+    DoNotRetryRegionException ex =
+      assertThrows(DoNotRetryRegionException.class, () -> am.assign(splitParentInfo),


Here you just call AssignmentManager.assign directly, my question is that, where do we call this in our code base? If this is not allowed, I prefer we use assert as obviously this is a code bug...

we can end up here from Admin.assign or Hbck.assigns. instead of calling AssignmentManager.assign directly, would you suggest that i write explicit tests for those paths?

went ahead and updated the tests to cover the Admin.assign and Hbck.assigns flow instead of calling AssignmentManager.assign. let me know if that wasn't what you had in mind, and i'll make adjustements.

The previous tests set the parent's state to SPLIT, which the existing expected-state check in createAssignProcedure already rejects, so they passed without the isSplit() guard. Rework them to use the case the guard actually decides: a parent loaded as CLOSED with regionInfo.isSplit()=true (a meta row written before this fix), and forced HBCK assigns, which skip the state check. Both are exercised through the client entry points, Admin.assign and Hbck.assigns. Also assert in TestRegionSplit that the split parent is in state SPLIT after a master restart. The existing isSplit() assertion there also passes with the old behavior, because regionInfo.isSplit() is true from the meta row, so it could not catch the state not being persisted. Drop the mini-cluster test that duplicated the meta write check already covered by TestRegionStateStore. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

Tanya Stickles and others added 3 commits September 24, 2026 11:18

updating comments

af158e1

adding another comment

ac031eb

tanyastickles commented Sep 24, 2026

View reviewed changes

Comment thread hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStateStore.java Resolved

tanyastickles commented Sep 24, 2026

View reviewed changes

tanyastickles changed the title ~~HBASE-30353 Prevent split parent regions from being re-opened after m…~~ HBASE-30353 Prevent split parent regions from being re-opened Sep 24, 2026

Tanya Stickles added 5 commits September 25, 2026 14:02

cleaning up -- removing comments from discussion

df28eda

removing more comments

a25c826

cleaning up tests

e07af65

adding some unit tests

7aec8bc

removing new line

02a2ce1

tanyastickles marked this pull request as ready for review September 25, 2026 19:14

Apache9 reviewed Sep 28, 2026

View reviewed changes

mvn spotless:apply

b69e473

tanyastickles requested review from Apache9 and wchevreuil October 1, 2026 17:35

wchevreuil approved these changes Oct 2, 2026

View reviewed changes

Apache9 reviewed Oct 6, 2026

View reviewed changes

charlesconnell approved these changes Oct 26, 2026

View reviewed changes

charlesconnell merged commit 71a127e into apache:master Oct 26, 2026
8 checks passed

Conversation

tanyastickles commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Apache9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Apache9 commented Sep 28, 2026

Uh oh!

tanyastickles commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Apache9 commented Sep 29, 2026

Uh oh!

tanyastickles commented Sep 29, 2026

Uh oh!

tanyastickles commented Sep 29, 2026

Uh oh!

wchevreuil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tanyastickles commented Sep 24, 2026 •

edited

Loading

tanyastickles commented Sep 28, 2026 •

edited

Loading