From 6d7140a482e90f01d3e94f5221367fde8170767b Mon Sep 17 00:00:00 2001 From: comnetwork Date: Wed, 7 Sep 2022 20:27:04 +0800 Subject: [PATCH 1/2] HBASE-27362 CompactSplit.requestCompactionInternal may bypass compactionsEnabled check --- .../hbase/regionserver/CompactSplit.java | 67 ++++++++++++++----- 1 file changed, 49 insertions(+), 18 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java index 1360a20c0f83..3e44855fafd8 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java @@ -229,8 +229,10 @@ private synchronized void requestSplit(final Region r, byte[] midKey) { */ private synchronized void requestSplit(final Region r, byte[] midKey, User user) { if (midKey == null) { - LOG.debug("Region " + r.getRegionInfo().getRegionNameAsString() - + " not splittable because midkey=null"); + if (LOG.isDebugEnabled()) { + LOG.debug("Region " + r.getRegionInfo().getRegionNameAsString() + + " not splittable because midkey=null"); + } return; } try { @@ -239,7 +241,9 @@ private synchronized void requestSplit(final Region r, byte[] midKey, User user) LOG.debug("Splitting " + r + ", " + this); } } catch (RejectedExecutionException ree) { - LOG.info("Could not execute split for " + r, ree); + if (LOG.isInfoEnabled()) { + LOG.info("Could not execute split for " + r, ree); + } } } @@ -312,14 +316,20 @@ public void switchCompaction(boolean onOrOff) { if (onOrOff) { // re-create executor pool if compactions are disabled. if (!isCompactionsEnabled()) { - LOG.info("Re-Initializing compactions because user switched on compactions"); + if (LOG.isInfoEnabled()) { + LOG.info("Re-Initializing compactions because user switched on compactions"); + } reInitializeCompactionsExecutors(); } - } else { - LOG.info("Interrupting running compactions because user switched off compactions"); - interrupt(); + setCompactionsEnabled(onOrOff); + return; } + setCompactionsEnabled(onOrOff); + if (LOG.isInfoEnabled()) { + LOG.info("Interrupting running compactions because user switched off compactions"); + } + interrupt(); } private void requestCompactionInternal(HRegion region, String why, int priority, @@ -336,6 +346,13 @@ private void requestCompactionInternal(HRegion region, String why, int priority, protected void requestCompactionInternal(HRegion region, HStore store, String why, int priority, boolean selectNow, CompactionLifeCycleTracker tracker, CompactionCompleteTracker completeTracker, User user) throws IOException { + if (!this.isCompactionsEnabled()) { + if (LOG.isInfoEnabled()) { + LOG.info("Ignoring compaction request for " + region + ",because compaction is disabled."); + } + return; + } + if ( this.server.isStopped() || (region.getTableDescriptor() != null && !region.getTableDescriptor().isCompactionEnabled()) @@ -355,7 +372,9 @@ protected void requestCompactionInternal(HRegion region, HStore store, String wh + " as an active space quota violation " + " policy disallows compactions."; tracker.notExecuted(store, reason); completeTracker.completed(store); - LOG.debug(reason); + if (LOG.isDebugEnabled()) { + LOG.debug(reason); + } return; } @@ -418,8 +437,10 @@ public void requestSystemCompaction(HRegion region, HStore store, String why) th public synchronized void requestSystemCompaction(HRegion region, HStore store, String why, boolean giveUpIfRequestedOrCompacting) throws IOException { if (giveUpIfRequestedOrCompacting && isUnderCompaction(store)) { - LOG.debug("Region {} store {} is under compaction now, skip to request compaction", region, - store.getColumnFamilyName()); + if (LOG.isDebugEnabled()) { + LOG.debug("Region {} store {} is under compaction now, skip to request compaction", region, + store.getColumnFamilyName()); + } return; } requestCompactionInternal(region, store, why, NO_PRIORITY, false, @@ -431,7 +452,9 @@ private Optional selectCompaction(HRegion region, HStore stor throws IOException { // don't even select for compaction if disableCompactions is set to true if (!isCompactionsEnabled()) { - LOG.info(String.format("User has disabled compactions")); + if (LOG.isInfoEnabled()) { + LOG.info(String.format("User has disabled compactions")); + } return Optional.empty(); } Optional compaction = store.requestCompaction(priority, tracker, user); @@ -459,7 +482,9 @@ private void waitFor(ThreadPoolExecutor t, String name) { while (!done) { try { done = t.awaitTermination(60, TimeUnit.SECONDS); - LOG.info("Waiting for " + name + " to finish..."); + if (LOG.isInfoEnabled()) { + LOG.info("Waiting for " + name + " to finish..."); + } if (!done) { t.shutdownNow(); } @@ -739,8 +764,10 @@ public void onConfigurationChange(Configuration newConf) { int largeThreads = Math.max(1, newConf.getInt(LARGE_COMPACTION_THREADS, LARGE_COMPACTION_THREADS_DEFAULT)); if (this.longCompactions.getCorePoolSize() != largeThreads) { - LOG.info("Changing the value of " + LARGE_COMPACTION_THREADS + " from " - + this.longCompactions.getCorePoolSize() + " to " + largeThreads); + if (LOG.isInfoEnabled()) { + LOG.info("Changing the value of " + LARGE_COMPACTION_THREADS + " from " + + this.longCompactions.getCorePoolSize() + " to " + largeThreads); + } if (this.longCompactions.getCorePoolSize() < largeThreads) { this.longCompactions.setMaximumPoolSize(largeThreads); this.longCompactions.setCorePoolSize(largeThreads); @@ -752,8 +779,10 @@ public void onConfigurationChange(Configuration newConf) { int smallThreads = newConf.getInt(SMALL_COMPACTION_THREADS, SMALL_COMPACTION_THREADS_DEFAULT); if (this.shortCompactions.getCorePoolSize() != smallThreads) { - LOG.info("Changing the value of " + SMALL_COMPACTION_THREADS + " from " - + this.shortCompactions.getCorePoolSize() + " to " + smallThreads); + if (LOG.isInfoEnabled()) { + LOG.info("Changing the value of " + SMALL_COMPACTION_THREADS + " from " + + this.shortCompactions.getCorePoolSize() + " to " + smallThreads); + } if (this.shortCompactions.getCorePoolSize() < smallThreads) { this.shortCompactions.setMaximumPoolSize(smallThreads); this.shortCompactions.setCorePoolSize(smallThreads); @@ -765,8 +794,10 @@ public void onConfigurationChange(Configuration newConf) { int splitThreads = newConf.getInt(SPLIT_THREADS, SPLIT_THREADS_DEFAULT); if (this.splits.getCorePoolSize() != splitThreads) { - LOG.info("Changing the value of " + SPLIT_THREADS + " from " + this.splits.getCorePoolSize() - + " to " + splitThreads); + if (LOG.isInfoEnabled()) { + LOG.info("Changing the value of " + SPLIT_THREADS + " from " + this.splits.getCorePoolSize() + + " to " + splitThreads); + } if (this.splits.getCorePoolSize() < splitThreads) { this.splits.setMaximumPoolSize(splitThreads); this.splits.setCorePoolSize(splitThreads); From 7b10b43da5c0445b09d22faca58636664ad53aa9 Mon Sep 17 00:00:00 2001 From: comnetwork Date: Thu, 8 Sep 2022 10:42:10 +0800 Subject: [PATCH 2/2] fix log --- .../hbase/regionserver/CompactSplit.java | 58 ++++++------------- 1 file changed, 17 insertions(+), 41 deletions(-) diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java index 3e44855fafd8..5b75d3414f1b 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java @@ -229,10 +229,8 @@ private synchronized void requestSplit(final Region r, byte[] midKey) { */ private synchronized void requestSplit(final Region r, byte[] midKey, User user) { if (midKey == null) { - if (LOG.isDebugEnabled()) { - LOG.debug("Region " + r.getRegionInfo().getRegionNameAsString() - + " not splittable because midkey=null"); - } + LOG.debug("Region " + r.getRegionInfo().getRegionNameAsString() + + " not splittable because midkey=null"); return; } try { @@ -241,9 +239,7 @@ private synchronized void requestSplit(final Region r, byte[] midKey, User user) LOG.debug("Splitting " + r + ", " + this); } } catch (RejectedExecutionException ree) { - if (LOG.isInfoEnabled()) { - LOG.info("Could not execute split for " + r, ree); - } + LOG.info("Could not execute split for " + r, ree); } } @@ -316,9 +312,7 @@ public void switchCompaction(boolean onOrOff) { if (onOrOff) { // re-create executor pool if compactions are disabled. if (!isCompactionsEnabled()) { - if (LOG.isInfoEnabled()) { - LOG.info("Re-Initializing compactions because user switched on compactions"); - } + LOG.info("Re-Initializing compactions because user switched on compactions"); reInitializeCompactionsExecutors(); } setCompactionsEnabled(onOrOff); @@ -326,9 +320,7 @@ public void switchCompaction(boolean onOrOff) { } setCompactionsEnabled(onOrOff); - if (LOG.isInfoEnabled()) { - LOG.info("Interrupting running compactions because user switched off compactions"); - } + LOG.info("Interrupting running compactions because user switched off compactions"); interrupt(); } @@ -347,9 +339,7 @@ protected void requestCompactionInternal(HRegion region, HStore store, String wh boolean selectNow, CompactionLifeCycleTracker tracker, CompactionCompleteTracker completeTracker, User user) throws IOException { if (!this.isCompactionsEnabled()) { - if (LOG.isInfoEnabled()) { - LOG.info("Ignoring compaction request for " + region + ",because compaction is disabled."); - } + LOG.info("Ignoring compaction request for " + region + ",because compaction is disabled."); return; } @@ -372,9 +362,7 @@ protected void requestCompactionInternal(HRegion region, HStore store, String wh + " as an active space quota violation " + " policy disallows compactions."; tracker.notExecuted(store, reason); completeTracker.completed(store); - if (LOG.isDebugEnabled()) { - LOG.debug(reason); - } + LOG.debug(reason); return; } @@ -437,10 +425,8 @@ public void requestSystemCompaction(HRegion region, HStore store, String why) th public synchronized void requestSystemCompaction(HRegion region, HStore store, String why, boolean giveUpIfRequestedOrCompacting) throws IOException { if (giveUpIfRequestedOrCompacting && isUnderCompaction(store)) { - if (LOG.isDebugEnabled()) { - LOG.debug("Region {} store {} is under compaction now, skip to request compaction", region, - store.getColumnFamilyName()); - } + LOG.debug("Region {} store {} is under compaction now, skip to request compaction", region, + store.getColumnFamilyName()); return; } requestCompactionInternal(region, store, why, NO_PRIORITY, false, @@ -452,9 +438,7 @@ private Optional selectCompaction(HRegion region, HStore stor throws IOException { // don't even select for compaction if disableCompactions is set to true if (!isCompactionsEnabled()) { - if (LOG.isInfoEnabled()) { - LOG.info(String.format("User has disabled compactions")); - } + LOG.info(String.format("User has disabled compactions")); return Optional.empty(); } Optional compaction = store.requestCompaction(priority, tracker, user); @@ -482,9 +466,7 @@ private void waitFor(ThreadPoolExecutor t, String name) { while (!done) { try { done = t.awaitTermination(60, TimeUnit.SECONDS); - if (LOG.isInfoEnabled()) { - LOG.info("Waiting for " + name + " to finish..."); - } + LOG.info("Waiting for " + name + " to finish..."); if (!done) { t.shutdownNow(); } @@ -764,10 +746,8 @@ public void onConfigurationChange(Configuration newConf) { int largeThreads = Math.max(1, newConf.getInt(LARGE_COMPACTION_THREADS, LARGE_COMPACTION_THREADS_DEFAULT)); if (this.longCompactions.getCorePoolSize() != largeThreads) { - if (LOG.isInfoEnabled()) { - LOG.info("Changing the value of " + LARGE_COMPACTION_THREADS + " from " - + this.longCompactions.getCorePoolSize() + " to " + largeThreads); - } + LOG.info("Changing the value of " + LARGE_COMPACTION_THREADS + " from " + + this.longCompactions.getCorePoolSize() + " to " + largeThreads); if (this.longCompactions.getCorePoolSize() < largeThreads) { this.longCompactions.setMaximumPoolSize(largeThreads); this.longCompactions.setCorePoolSize(largeThreads); @@ -779,10 +759,8 @@ public void onConfigurationChange(Configuration newConf) { int smallThreads = newConf.getInt(SMALL_COMPACTION_THREADS, SMALL_COMPACTION_THREADS_DEFAULT); if (this.shortCompactions.getCorePoolSize() != smallThreads) { - if (LOG.isInfoEnabled()) { - LOG.info("Changing the value of " + SMALL_COMPACTION_THREADS + " from " - + this.shortCompactions.getCorePoolSize() + " to " + smallThreads); - } + LOG.info("Changing the value of " + SMALL_COMPACTION_THREADS + " from " + + this.shortCompactions.getCorePoolSize() + " to " + smallThreads); if (this.shortCompactions.getCorePoolSize() < smallThreads) { this.shortCompactions.setMaximumPoolSize(smallThreads); this.shortCompactions.setCorePoolSize(smallThreads); @@ -794,10 +772,8 @@ public void onConfigurationChange(Configuration newConf) { int splitThreads = newConf.getInt(SPLIT_THREADS, SPLIT_THREADS_DEFAULT); if (this.splits.getCorePoolSize() != splitThreads) { - if (LOG.isInfoEnabled()) { - LOG.info("Changing the value of " + SPLIT_THREADS + " from " + this.splits.getCorePoolSize() - + " to " + splitThreads); - } + LOG.info("Changing the value of " + SPLIT_THREADS + " from " + this.splits.getCorePoolSize() + + " to " + splitThreads); if (this.splits.getCorePoolSize() < splitThreads) { this.splits.setMaximumPoolSize(splitThreads); this.splits.setCorePoolSize(splitThreads);