[GitHub] [hbase] virajjasani commented on a change in pull request #1990: HBASE-24648 Remove the legacy 'forceSplit' related code at region ser…

2020-06-29 Thread GitBox 


virajjasani commented on a change in pull request #1990:
URL: https://github.com/apache/hbase/pull/1990#discussion_r446834036



##
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:
   Ok, as far as current checkstyle is fine, 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

[GitHub] [hbase] virajjasani commented on a change in pull request #1990: HBASE-24648 Remove the legacy 'forceSplit' related code at region ser…

2020-06-29 Thread GitBox 


virajjasani commented on a change in pull request #1990:
URL: https://github.com/apache/hbase/pull/1990#discussion_r446797209



##
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:
   Oh ok, then we can write description of IOException as `thrown if region 
is not splittable`?
   I thought spotbugs would give warning if we remove @throws from javadoc but 
method actually throws Exception.
   Anyways, this is minor comment.





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

[GitHub] [hbase] virajjasani commented on a change in pull request #1990: HBASE-24648 Remove the legacy 'forceSplit' related code at region ser…

2020-06-28 Thread GitBox 


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()` as well as 
`node.getRegionLocation()` with this?





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

[GitHub] [hbase] virajjasani commented on a change in pull request #1990: HBASE-24648 Remove the legacy 'forceSplit' related code at region ser…

2020-06-28 Thread GitBox 


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()` as well as 
`node.getRegionLocation()` with this line?





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

[GitHub] [hbase] virajjasani commented on a change in pull request #1990: HBASE-24648 Remove the legacy 'forceSplit' related code at region ser…

2020-06-28 Thread GitBox 


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 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 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