wchevreuil commented on a change in pull request #2011: URL: https://github.com/apache/hbase/pull/2011#discussion_r450757746
########## 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 are breaking isolation and cohesion principles, this method should log only the info it has locally, without requiring any additional param just for logging. The additional info available in eventual callers should then be logged by those callers, not passed through as params, here I suggest the log to be only: `LOG.debug("ShouldSplit because region size is big enough " + "size={}, sizeToCheck={}", StringUtils.humanSize(sumSize), StringUtils.humanSize(sizeToCheck))` Then on IncreasingToUpperBoundRegionSplitPolicy you log the extra info on an additional debug message: `LOG.debug(regionsWithCommonTable={}", tableRegionsCount)` ########## 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: >Make the logic more clear. If can not split, then we do not need to get tableRegionsCount and sizeToCheck in IncreasingToUpperBoundRegionSplitPolicy, and also avoid other comparsion in loop too. That's a problem for IncreasingToUpperBoundRegionSplitPolicy. We shouldn't penalise a parent class because of subclasses implementation specifics. And you are already overriding this method on IncreasingToUpperBoundRegionSplitPolicy to do the extra loop anyways, so why keep this extra, needless loop here? ---------------------------------------------------------------- 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