bbeaudreault commented on code in PR #5534:
URL: https://github.com/apache/hbase/pull/5534#discussion_r1408145896
##########
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:
Let's look back at the original impl of CONFIRM_REOPEN. It only does the
backoff if there are still regions to reopen and _none of them are
schedulable_. Meaning we back off in order to wait for regions to be
schedulable.
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_.
The key thing there is that "_process next batch_". You'd think that would
be "re-schedule existing batch". But as is, when you go to process next batch,
the first thing REOPEN_REGIONS does is create a new currentRegionBatch. So lets
say for the first batch of 50, only 10 reopen. With the current design, you'd
expect it to try again to reopen those remaining 40. But it will actually
reopen another 50 (40 of which are likely to have been in the last batch).
TBH I don't think there's a huge problem with that approach. But given
that's how it works, we can simplify this code as I described before -- we
don't really care about currentRegionBatch here, just that if any regions are
left to reopen and schedulable.
If we wanted to do what I think you are intending to do, we probably need to
complicate things a bit further. Maybe we update REOPEN_REGIONS to only create
currentRegionBatch if it's currently null. Then in CONFIRM_REOPEN, null it out
after confirming that they've been reopened. That way, in the above example
when we kickoff REOPEN_REGIONS again we'll only open the remaining 40.
Does that make sense? Between these 2 options I might opt for the simpler
one where we keep REOPEN_REGIONS as is and remove most of the diff from
CONFIRM_REOPEN.
--
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]
