Skip to content

HBASE-27362 CompactSplit.requestCompactionInternal may bypass compact… #4768

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Apache9 merged 2 commits into from
Sep 8, 2022
Merged

HBASE-27362 CompactSplit.requestCompactionInternal may bypass compact… #4768

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d7140a
HBASE-27362 CompactSplit.requestCompactionInternal may bypass compact…
comnetwork Sep 7, 2022
7b10b43
fix log
comnetwork Sep 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 49 additions & 18 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Copy link
Contributor

@Apache9 Apache9 Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these logging changes related?

Copy link
Contributor Author

@comnetwork comnetwork Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it is not related, just modified by the way.

LOG.debug("Region " + r.getRegionInfo().getRegionNameAsString()
+ " not splittable because midkey=null");
}
return;
}
try {
Expand All @@ -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);
}
}
}

Expand Down Expand Up @@ -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()) {
Copy link
Contributor

@Apache9 Apache9 Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer we keep the old style.

Copy link
Contributor Author

@comnetwork comnetwork Sep 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I have reverted it.

Copy link
Contributor

@Apache9 Apache9 Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to align them, I prefer we open a new issue.

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,
Expand All @@ -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()) {
Copy link
Contributor

@Apache9 Apache9 Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the actual fix?

Copy link
Contributor Author

@comnetwork comnetwork Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it is the actual fix.

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())
Expand All @@ -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;
}

Expand Down Expand Up @@ -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,
Expand All @@ -431,7 +452,9 @@ private Optional<CompactionContext> 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<CompactionContext> compaction = store.requestCompaction(priority, tracker, user);
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down