junegunn commented on code in PR #6797:
URL: https://github.com/apache/hbase/pull/6797#discussion_r2103712873
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java:
##########
@@ -392,10 +399,26 @@ private void closeRegion(MasterProcedureEnv env,
RegionStateNode regionNode)
) {
return;
}
- if (regionNode.isInState(State.OPEN, State.CLOSING, State.MERGING,
State.SPLITTING)) {
- // this is the normal case
- ProcedureFutureUtil.suspendIfNecessary(this, this::setFuture,
- env.getAssignmentManager().regionClosing(regionNode), env,
+
+ CompletableFuture<Void> future = null;
+ if (regionNode.isInState(STATES_EXPECTED_ON_CLOSING)) {
+ // This is the normal case
+ future = env.getAssignmentManager().regionClosing(regionNode);
+ } else if (regionNode.setState(State.CLOSED, State.FAILED_OPEN)) {
+ // FAILED_OPEN doesn't need further transition, immediately mark the
region as closed
+ AssignmentManager am = env.getAssignmentManager();
+ am.getRegionStates().removeFromFailedOpen(regionNode.getRegionInfo());
+ future = am.getRegionStateStore().updateRegionLocation(regionNode);
Review Comment:
> And if the state is CLOSED, do we still need to call udpateRegionLocation?
I called the method to persist the in-memory change of `regionNode` to the
meta table. Would it be clearer if I call `persistToMeta` instead? The result
is roughly the same.
```diff
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java
index 6b5aaf6975..69c9d1ffca 100644
---
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java
+++
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java
@@ -408,7 +408,7 @@ public class TransitRegionStateProcedure
// FAILED_OPEN doesn't need further transition, immediately mark the
region as closed
AssignmentManager am = env.getAssignmentManager();
am.getRegionStates().removeFromFailedOpen(regionNode.getRegionInfo());
- future = am.getRegionStateStore().updateRegionLocation(regionNode);
+ future = am.persistToMeta(regionNode);
}
if (future != null) {
ProcedureFutureUtil.suspendIfNecessary(this, this::setFuture, future,
env,
```
If the question is about if it's necessary to persist the changed state to
meta, it looks like it's not necessary in this case, though we have to change
an assertion in the test code.
```diff
diff --git
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java
index 6b5aaf6975..6b7989dd58 100644
---
a/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java
+++
b/hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/TransitRegionStateProcedure.java
@@ -408,7 +408,7 @@ public class TransitRegionStateProcedure
// FAILED_OPEN doesn't need further transition, immediately mark the
region as closed
AssignmentManager am = env.getAssignmentManager();
am.getRegionStates().removeFromFailedOpen(regionNode.getRegionInfo());
- future = am.getRegionStateStore().updateRegionLocation(regionNode);
+ future = CompletableFuture.allOf();
}
if (future != null) {
ProcedureFutureUtil.suspendIfNecessary(this, this::setFuture, future,
env,
diff --git
a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestTransitRegionStateProcedure.java
b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestTransitRegionStateProcedure.java
index 074e7e730c..392993280b 100644
---
a/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestTransitRegionStateProcedure.java
+++
b/hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestTransitRegionStateProcedure.java
@@ -250,8 +250,8 @@ public class TestTransitRegionStateProcedure {
// The number of RITs should be 0 after disabling the table
assertEquals(0, getTotalRITs());
- // The regions are now in "CLOSED" state
- assertEquals(Collections.singleton("CLOSED"), getRegionStates());
+ // The regions are still in "FAILED_OPEN" state
+ assertEquals(Collections.singleton("FAILED_OPEN"), getRegionStates());
// Fix the error in the table descriptor
tdb = TableDescriptorBuilder.newBuilder(td);
```
However, I think we should try to keep the in-memory state and persistent
meta state synchronized to avoid confusion and any potential issues that might
arise.
--
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]
