bbeaudreault commented on code in PR #5534:
URL: https://github.com/apache/hbase/pull/5534#discussion_r1409395998
##########
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:
Ok lets table the missing outcomes/simplification discussion in that case,
since we'll want to keep the current logic for the progressive reopen. We'll
also want to do as i said, where in CONFIRM we update currentRegionBatch to
filter out the reopened regions, and then in REOPEN we should only create a new
currentRegionBatch if the existing isEmpty. That way if there's some
problematic region in the batch, we won't move on to the next batch until its
been reopened. Right now I think we'd just move on and create a new batch
including the problematic region + more?
Once we have that in there, I need to take another look at the logic here. I
think it could be a bit clearer, but don't want to make any suggestions until
we have the complete impl in place.
In terms of progressive, it'd be nice to start with 1 region but otherwise
agree with your Math.min idea.
--
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]
