rmdmattingly commented on code in PR #5534:
URL: https://github.com/apache/hbase/pull/5534#discussion_r1409355128
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ReopenTableRegionsProcedure.java:
##########
@@ -139,33 +170,57 @@ protected Flow executeFromState(MasterProcedureEnv env,
ReopenTableRegionsState
case REOPEN_TABLE_REGIONS_CONFIRM_REOPENED:
regions =
regions.stream().map(env.getAssignmentManager().getRegionStates()::checkReopened)
.filter(l -> l != null).collect(Collectors.toList());
- if (regions.isEmpty()) {
- return Flow.NO_MORE_STATE;
+ // we need to create a set of region names because the HRegionLocation
hashcode is only
+ // based
+ // on the server name
+ Set<byte[]> currentRegionBatchNames = currentRegionBatch.stream()
+ .map(r -> r.getRegion().getRegionName()).collect(Collectors.toSet());
+ currentRegionBatch = regions.stream()
+ .filter(r ->
currentRegionBatchNames.contains(r.getRegion().getRegionName()))
+ .collect(Collectors.toList());
+ if (currentRegionBatch.isEmpty()) {
+ if (regions.isEmpty()) {
+ return Flow.NO_MORE_STATE;
+ } else {
+
setNextState(ReopenTableRegionsState.REOPEN_TABLE_REGIONS_REOPEN_REGIONS);
+ if (reopenBatchBackoffMillis > 0) {
+ backoff(reopenBatchBackoffMillis);
+ }
+ return Flow.HAS_MORE_STATE;
+ }
}
- if (regions.stream().anyMatch(loc -> canSchedule(env, loc))) {
+ if (currentRegionBatch.stream().anyMatch(loc -> canSchedule(env,
loc))) {
Review Comment:
> Now let's look at your new code. I see what you are saying. You are
basically doing:
If currentRegionBatch are all reopened, exit or process the next batch
Otherwise, can any of the remaining be scheduled? If not, exponential
backoff. If so, process next batch.
I think that's missing a few distinct outcomes of `CONFIRM_REOPENED`, and
that's where this complexity comes into play. Here are the possibilities:
1. The current batch has reopened successfully and there are no more regions
to produce a next batch, so finish
2. The current batch has reopened successfully and there are more regions to
produce a next batch, so set the next state to `REOPEN_REGIONS` and suspend.
This will process the next batch
3. The current batch has not reopened successfully, and some of the current
batch can still be scheduled. In this case return to `REOPEN_REGIONS` to avoid
an infinite loop due to a bypassed proc or something. This could technically
reprocess the next batch, or redefine what the "current batch" looks like, but
I think that's ok given the exceptional nature of this case
4. The current batch has not reopened successfully, and none of the current
batch can still be scheduled (because they have already been scheduled). In
this case we're just waiting on TRSPs to complete, so retain current state of
`CONFIRM_REOPENED` and retry w/ increasing backoff
If I just revert this diff as described then this procedure will just get
stuck in infinite loops (that I think could theoretically be exited if TRSP
finishes quickly enough?) and all unit tests fail
> We could also trivially update this code to do an actual progressive
deploy -- first batch size 1, then 2, then 4, etc up to the current batch size
config you added. At that point it stays at that max concurrency until
completion.
I like this idea, and I think it would be trivial to add in the current
state of this PR. We'd just need to store a maxReopenBatchSize and increase the
reopenBatchSize to Math.min(2*reopenBatchSize, maxReopenBatchSize) each time we
enter state 2 above
--
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]
