virajjasani commented on a change in pull request #1990:
URL: https://github.com/apache/hbase/pull/1990#discussion_r446620241
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java
##########
@@ -187,19 +190,19 @@ private void checkSplittable(final MasterProcedureEnv env,
boolean splittable = false;
if (node != null) {
try {
- if (bestSplitRow == null || bestSplitRow.length == 0) {
+ GetRegionInfoResponse response;
+ if (!hasBestSplitRow()) {
LOG
.info("splitKey isn't explicitly specified, will try to find a
best split key from RS");
Review comment:
Good to log `node.getRegionInfo().getRegionNameAsString()` with this?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/SplitTableRegionProcedure.java
##########
@@ -167,12 +167,15 @@ public RegionInfo getDaughterTwoRI() {
return daughterTwoRI;
}
+ private boolean hasBestSplitRow() {
+ return bestSplitRow != null && bestSplitRow.length > 0;
+ }
+
/**
* Check whether the region is splittable
* @param env MasterProcedureEnv
* @param regionToSplit parent Region to be split
* @param splitRow if splitRow is not specified, will first try to get
bestSplitRow from RS
- * @throws IOException
Review comment:
Seems unintentional change?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
##########
@@ -8537,49 +8535,25 @@ public void run(Message message) {
return responseBuilder.build();
}
- boolean shouldForceSplit() {
- return this.splitRequest;
- }
-
- byte[] getExplicitSplitPoint() {
- return this.explicitSplitPoint;
- }
-
- void forceSplit(byte[] sp) {
- // This HRegion will go away after the forced split is successful
- // But if a forced split fails, we need to clear forced split.
- this.splitRequest = true;
- if (sp != null) {
- this.explicitSplitPoint = sp;
- }
- }
-
- void clearSplit() {
- this.splitRequest = false;
- this.explicitSplitPoint = null;
+ public Optional<byte[]> checkSplit() {
+ return checkSplit(false);
}
/**
- * Return the splitpoint. null indicates the region isn't splittable
- * If the splitpoint isn't explicitly specified, it will go over the stores
- * to find the best splitpoint. Currently the criteria of best splitpoint
- * is based on the size of the store.
+ * Return the split point. An empty result indicates the region isn't
splittable.
*/
- public byte[] checkSplit() {
+ public Optional<byte[]> checkSplit(boolean force) {
// Can't split META
if (this.getRegionInfo().isMetaRegion()) {
- if (shouldForceSplit()) {
- LOG.warn("Cannot split meta region in HBase 0.20 and above");
- }
- return null;
+ return Optional.empty();
}
// Can't split a region that is closing.
if (this.isClosing()) {
- return null;
+ return Optional.empty();
}
- if (!splitPolicy.shouldSplit()) {
+ if (!force && !splitPolicy.shouldSplit()) {
return null;
Review comment:
Better to return `Optional.empty()`
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
##########
@@ -1837,35 +1837,30 @@ public GetOnlineRegionResponse getOnlineRegion(final
RpcController controller,
@Override
@QosPriority(priority=HConstants.ADMIN_QOS)
public GetRegionInfoResponse getRegionInfo(final RpcController controller,
- final GetRegionInfoRequest request) throws ServiceException {
+ final GetRegionInfoRequest request) throws ServiceException {
try {
checkOpen();
requestCount.increment();
HRegion region = getRegion(request.getRegion());
RegionInfo info = region.getRegionInfo();
- byte[] bestSplitRow = null;
- boolean shouldSplit = true;
+ byte[] bestSplitRow;
if (request.hasBestSplitRow() && request.getBestSplitRow()) {
- HRegion r = region;
- region.startRegionOperation(Operation.SPLIT_REGION);
- r.forceSplit(null);
- // Even after setting force split if split policy says no to split
then we should not split.
- shouldSplit = region.getSplitPolicy().shouldSplit() &&
!info.isMetaRegion();
Review comment:
After this change, `region.getSplitPolicy()` is no longer read by source
code, only test code. If test code can get splitPolicy using reflection, maybe
we can remove `getSplitPolicy()` from HRegion. Anyways, not a strong preference
w.r.t this PR, we are good.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
