[GitHub] [hbase] Apache9 commented on a change in pull request #2011: HBASE-24664 Some changing of split region by overall region size rath…

[GitBox <gi...@apache.org>]

Apache9 commented on a change in pull request #2011:
URL: https://github.com/apache/hbase/pull/2011#discussion_r453313685



##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
##########
@@ -382,6 +382,13 @@
   /** Default maximum file size */
   public static final long DEFAULT_MAX_FILE_SIZE = 10 * 1024 * 1024 * 1024L;
 
+  /** Conf key for if we should sum overall region files size when check to split */
+  public static final String OVERALL_HREGION_FILES =

Review comment:
       Should we use 'hregion' or 'region'? I'm not sure, just ask.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ConstantSizeRegionSplitPolicy.java
##########
@@ -38,10 +41,13 @@
  */
 @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.CONFIG)
 public class ConstantSizeRegionSplitPolicy extends RegionSplitPolicy {
+  private static final Logger LOG =
+    LoggerFactory.getLogger(ConstantSizeRegionSplitPolicy.class);
   private static final Random RANDOM = new Random();
 
   private long desiredMaxFileSize;
   private double jitterRate;
+  protected boolean overallHregionFiles;

Review comment:
       overallHRegionFiles? The 'R' should be upper case.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionSplitPolicy.java
##########
@@ -75,7 +75,7 @@ protected void configureForRegion(HRegion region) {
    */
   protected boolean canSplit() {
     return !region.getRegionInfo().isMetaRegion() && region.isAvailable() &&
-      !region.hasReferences();
+      region.getStores().stream().allMatch(HStore::canSplit);

Review comment:
       This is an interesting change. Is behavior still the same?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ConstantSizeRegionSplitPolicy.java
##########
@@ -94,4 +90,33 @@ long getDesiredMaxFileSize() {
   public boolean positiveJitterRate() {
     return this.jitterRate > 0;
   }
+
+  /**
+   * @return true if region size exceed the sizeToCheck
+   */
+  protected boolean isExceedSize(long sizeToCheck) {

Review comment:
       Should we make this method final? I think it is used to be called by sub classes, not to be overridden?




----------------------------------------------------------------
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:
users@infra.apache.org