bbeaudreault commented on code in PR #5534:
URL: https://github.com/apache/hbase/pull/5534#discussion_r1412589480
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ReopenTableRegionsProcedure.java:
##########
@@ -61,20 +73,43 @@ public class ReopenTableRegionsProcedure
private List<HRegionLocation> regions = Collections.emptyList();
+ private List<HRegionLocation> currentRegionBatch = Collections.emptyList();
+
private RetryCounter retryCounter;
+ private long reopenBatchBackoffMillis;
+ private int reopenBatchSize;
+ private int reopenBatchSizeMax;
+ private long regionsReopened = 0;
+ private long batchesProcessed = 0;
+
public ReopenTableRegionsProcedure() {
- regionNames = Collections.emptyList();
+ this(null);
}
public ReopenTableRegionsProcedure(TableName tableName) {
- this.tableName = tableName;
- this.regionNames = Collections.emptyList();
+ this(tableName, Collections.emptyList());
}
public ReopenTableRegionsProcedure(final TableName tableName, final
List<byte[]> regionNames) {
+ this(tableName, regionNames, PROGRESSIVE_BATCH_BACKOFF_MILLIS_DEFAULT,
+ PROGRESSIVE_BATCH_SIZE_MAX_DISABLED);
+ }
+
+ public ReopenTableRegionsProcedure(final TableName tableName, long
reopenBatchBackoffMillis,
+ int reopenBatchSizeMax) {
+ this(tableName, Collections.emptyList(), reopenBatchBackoffMillis,
reopenBatchSizeMax);
+ }
+
+ public ReopenTableRegionsProcedure(final TableName tableName, final
List<byte[]> regionNames,
+ long reopenBatchBackoffMillis, int reopenBatchSizeMax) {
this.tableName = tableName;
this.regionNames = regionNames;
+ this.reopenBatchBackoffMillis = reopenBatchBackoffMillis;
+ this.reopenBatchSize = reopenBatchSizeMax !=
PROGRESSIVE_BATCH_SIZE_MAX_DISABLED
+ ? 1
+ : PROGRESSIVE_BATCH_SIZE_MAX_DEFAULT_VALUE;
+ this.reopenBatchSizeMax = Math.max(reopenBatchSizeMax,
MINIMUM_BATCH_SIZE_MAX);
Review Comment:
sorry, this is now a little confusing. at first i thought this was a bug. if
reopenBatchSizeMax was -1, we'd set the batchSize to int.max, and
reopenBatchSizeMax to 1. so then below in our batch handling in CONFIRM_REGIONS
we'd reset reopenBatchSize to 1 because of the Math.min.... but then I
realized, since we use Int.Max it wouldn't go into the code. anyway, it might
just be a bit cleaner to do:
```
if (reopenBatchSizeMax == PROGRESSIVE_BATCH_SIZE_MAX_DISABLED) {
this.reopenBatchSize = Integer.MAX_VALUE;
this.reopenBatchSizeMax = Integer.MAX_VALUE;
} else {
this.reopenBatchSize = 1;
this.reopenBatchSizeMax = Math.max(reopenBatchSizeMax,
MINIMUM_BATCH_SIZE_MAX);
}
```
And now that I look again below, I actually _do_ think this is a bug. Let's
say batch is disabled, and REOPEN_REGIONS tries to reopen all regions at once.
But then in CONFIRM_REGIONS some of them did not open. We'd fall into the
canSchedule block and `shouldBatchBackoff` would be true. So then we'd do
`Math.min(reopenBatchSizeMax, 2 * Integer.Max)`. I actually think the 2 * max
would overflow negative and who knows what would happen.
So maybe we should do the clarification above, but we also need to make sure
we don't overflow below
--
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]
