-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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.
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
HBASE-27920:Skipping compact for this region if the table disable compaction #5273
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a bug, or are we actually skipping compaction later on?
Jira says it affects 2.3 and 2.4, but this PR is intended to master. Can you clarify which actual versions are affected and make sure we target the correct branches?
Please add UT for this extra condition.
It is not a bug and would skip compaction later. see code in here: Skipping compaction if we donot need it In here, I mean we only skip early before we foreach |
Sorry for this. What I described here is inaccurate. |
I think it is difficult,beacause I just skip early for avoiding useless loop later. We can get information whether the table enable compaction by calling |
Please add more comments to say that this is just for skipping compaction earlier, and we also have other checks in other places. |
@@ -1638,8 +1638,8 @@ private static class CompactionChecker extends ScheduledChore { | |||
@Override | |||
protected void chore() { | |||
for (Region r : this.instance.onlineRegions.values()) { | |||
// Skip compaction if region is read only | |||
if (r == null || r.isReadOnly()) { | |||
// Skip compaction if region is read only or table disable compaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be more clear:
// If region is read only or compaction is disabled at table level, there's no need to iterate through region's stores
Thanks for explaining it. Makes sense to me, approved this, just had a suggestion to "reword" the comment. |
In here,chore() is periodically called,The code call process is that:
And, There are no checks for compaction before calling chore()
Yes, there are checks when iterating through region's stores |
Thanks for your comments, I will update later |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
The failed UT is not related. Let me merge. |
…paction (#5273) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Wellington Chevreuil <[email protected]> (cherry picked from commit d1f29d0)
…paction (#5273) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Wellington Chevreuil <[email protected]> (cherry picked from commit d1f29d0)
…paction (#5273) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Wellington Chevreuil <[email protected]> (cherry picked from commit d1f29d0)
…paction (#5273) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Wellington Chevreuil <[email protected]> (cherry picked from commit d1f29d0)
…paction (apache#5273) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Wellington Chevreuil <[email protected]> (cherry picked from commit d1f29d0) (cherry picked from commit 94f8424) Change-Id: Id79569919da544c90a473a2c2987420461909218
Details see: HBASE-27920