wchevreuil commented on a change in pull request #2011: URL: https://github.com/apache/hbase/pull/2011#discussion_r450139888
########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ConstantSizeRegionSplitPolicy.java ########## @@ -68,22 +76,14 @@ protected void configureForRegion(HRegion region) { @Override protected boolean shouldSplit() { - boolean foundABigStore = false; - + // If any of the stores is unable to split (eg they contain reference files) + // then don't split for (HStore store : region.getStores()) { - // If any of the stores are unable to split (eg they contain reference files) - // then don't split - if ((!store.canSplit())) { + if (!store.canSplit()) { Review comment: Move this check to the for loops inside _isExceedSize_, so that we don't have to do do an extra iteration for all stores again in case none returns false for _canSplit_. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ConstantSizeRegionSplitPolicy.java ########## @@ -94,4 +94,33 @@ long getDesiredMaxFileSize() { public boolean positiveJitterRate() { return this.jitterRate > 0; } + + /** + * @return true if region size exceed the sizeToCheck + */ + protected boolean isExceedSize(long sizeToCheck, String extraLogStr) { + if (overallHregionFiles) { + long sumSize = 0; + for (HStore store : region.getStores()) { + sumSize += store.getSize(); + } + if (sumSize > sizeToCheck) { Review comment: We should just return this comparison and let each caller decide how to log it? That would discard the need for having an extra param just for the sake of logging. ---------------------------------------------------------------- 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