You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2022/04/13 14:46:47 UTC
[GitHub] [bookkeeper] hangc0276 opened a new pull request, #3205: Fix force GC doesn't work under forceAllowCompaction when disk is full
hangc0276 opened a new pull request, #3205:
URL: https://github.com/apache/bookkeeper/pull/3205
## Motivation
When I set `forceAllowCompaction=true` and one ledger disk reaches max usage threshold and transfer bookie to readOnly mode, I expire some pulsar topics or delete some topics to free up disk space. I found that ledger compression cannot be triggered when using `curl -XPUT http://localhost:8000/api/v1/bookie/gc` command.
The root cause is that when one ledger disk reaches max usage threshold, it will suspend minor and major compaction
https://github.com/apache/bookkeeper/blob/f7579fd13d62ce630ea26638e73f5884da505ec8/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java#L1041-L1058
When we use `curl -XPUT http://localhost:8000/api/v1/bookie/gc` command to trigger compaction, it will be filtered by `suspendMajor` and `suspendMinor` flag.
https://github.com/apache/bookkeeper/blob/f7579fd13d62ce630ea26638e73f5884da505ec8/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java#L416-L444
It will lead to
- The bookie won't clean up deleted ledgers
- Ledger disk can't free up disk usage
- Bookie can't recover from readOnly state into Writeable state.
And then we can only trigger compaction by the following steps.
- Increase max disk usage threshold
- Restart the bookie
- Use command `curl -XPUT http://localhost:8000/api/v1/bookie/gc` to trigger compaction
### Changes
1. Don't take the `suspendMajor` and `suspendMinor` flag into consideration when setting `forceAllowCompaction=true` and triggered by force GC.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #3205: Fix force GC doesn't work under forceAllowCompaction when disk is full
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on code in PR #3205:
URL: https://github.com/apache/bookkeeper/pull/3205#discussion_r850379773
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java:
##########
@@ -332,6 +332,95 @@ public void checkpointComplete(Checkpoint checkPoint, boolean compact)
});
}
+ @Test
+ public void testForceGarbageCollectionWhenDiskIsFull() throws Exception {
+ testForceGarbageCollectionWhenDiskIsFull(true);
+ testForceGarbageCollectionWhenDiskIsFull(false);
+ }
+
+ public void testForceGarbageCollectionWhenDiskIsFull(boolean isForceCompactionAllowWhenDisableCompaction)
+ throws Exception {
+
+ restartBookies(conf -> {
+ if (isForceCompactionAllowWhenDisableCompaction) {
+ conf.setMinorCompactionInterval(0);
+ conf.setMajorCompactionInterval(0);
+ conf.setForceAllowCompaction(true);
+ conf.setMajorCompactionThreshold(0.5f);
+ conf.setMinorCompactionThreshold(0.2f);
+ } else {
+ conf.setMinorCompactionInterval(120000);
Review Comment:
I just call the `triggerGC` to trigger compaction, so the interval won't affect the time cost of the test run.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3205: Fix force GC doesn't work under forceAllowCompaction when disk is full
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3205:
URL: https://github.com/apache/bookkeeper/pull/3205#issuecomment-1098624372
> Looks like it's expected behavior? We have another configuration `isForceGCAllowWhenNoSpace` which is used to control the force gc is allowed when no space, maybe you need to set it to true?
@zymap If we set `isForceGCAllowWhenNoSpace` to `true`, it won't suspend minor and major compaction on GC, which will easily lead to ledger disk space exhaustion. After we delete topic or shorten the retention to release disk space, we can't trigger compaction due to compaction need another disk space and the current disk space has been exhausted. So we are not recommend to set `isForceGCAllowWhenNoSpace` to `true`
What we need is that the bookie suspend the minor and major compaction when disk usage reaches max threshold, but when we deleted topic or shorten the retention, we trigger GC by rest api, and bookie server neglect the current `suspendMajor` and `suspendMinor` flag to trigger compaction.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3205: Fix force GC doesn't work under forceAllowCompaction when disk is full
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3205:
URL: https://github.com/apache/bookkeeper/pull/3205#issuecomment-1099071345
> Could we add unit tests?
@nicoloboschi I have added the unit test, please take a look, thanks.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3205: Fix force GC doesn't work under forceAllowCompaction when disk is full
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3205:
URL: https://github.com/apache/bookkeeper/pull/3205#issuecomment-1099378474
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #3205: Fix force GC doesn't work under forceAllowCompaction when disk is full
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on code in PR #3205:
URL: https://github.com/apache/bookkeeper/pull/3205#discussion_r851852186
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java:
##########
@@ -332,6 +332,95 @@ public void checkpointComplete(Checkpoint checkPoint, boolean compact)
});
}
+ @Test
+ public void testForceGarbageCollectionWhenDiskIsFull() throws Exception {
+ testForceGarbageCollectionWhenDiskIsFull(true);
+ testForceGarbageCollectionWhenDiskIsFull(false);
+ }
+
+ public void testForceGarbageCollectionWhenDiskIsFull(boolean isForceCompactionAllowWhenDisableCompaction)
+ throws Exception {
+
+ restartBookies(conf -> {
+ if (isForceCompactionAllowWhenDisableCompaction) {
+ conf.setMinorCompactionInterval(0);
+ conf.setMajorCompactionInterval(0);
+ conf.setForceAllowCompaction(true);
+ conf.setMajorCompactionThreshold(0.5f);
+ conf.setMinorCompactionThreshold(0.2f);
+ } else {
+ conf.setMinorCompactionInterval(120000);
+ conf.setMajorCompactionInterval(240000);
+ }
+ return conf;
+ });
+
+ getGCThread().suspendMajorGC();
+ getGCThread().suspendMinorGC();
+ long majorCompactionCntBeforeGC = 0;
+ long minorCompactionCntBeforeGC = 0;
+ long majorCompactionCntAfterGC = 0;
+ long minorCompactionCntAfterGC = 0;
+
+ // disable forceMajor and forceMinor
+ majorCompactionCntBeforeGC = getGCThread().getGarbageCollectionStatus().getMajorCompactionCounter();
+ minorCompactionCntBeforeGC = getGCThread().getGarbageCollectionStatus().getMinorCompactionCounter();
+ getGCThread().triggerGC(true, true, true).get();
+ majorCompactionCntAfterGC = getGCThread().getGarbageCollectionStatus().getMajorCompactionCounter();
+ minorCompactionCntAfterGC = getGCThread().getGarbageCollectionStatus().getMinorCompactionCounter();
+ if (isForceCompactionAllowWhenDisableCompaction) {
Review Comment:
Whether we will enter major or minor compaction is controlled by
```
((isForceMajorCompactionAllow && force) || (enableMajorCompaction
&& (force || curTime - lastMajorCompactionTime > majorCompactionInterval)))
&& (!suspendMajor)
```
That is:
1. `isForceMajorCompactionAllow && force` or `(enableMajorCompaction && (force || curTime - lastMajorCompactionTime > majorCompactionInterval)`
2. suspendMajor
We must meet above both conditions.
The if and else is different code path controlled by `isForceMajorCompactionAllow && force` and `(enableMajorCompaction && (force || curTime - lastMajorCompactionTime > majorCompactionInterval)`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3205: Fix force GC doesn't work under forceAllowCompaction when disk is full
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3205:
URL: https://github.com/apache/bookkeeper/pull/3205#issuecomment-1099183939
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] zymap merged pull request #3205: Fix force GC doesn't work under forceAllowCompaction when disk is full
Posted by GitBox <gi...@apache.org>.
zymap merged PR #3205:
URL: https://github.com/apache/bookkeeper/pull/3205
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3205: Fix force GC doesn't work under forceAllowCompaction when disk is full
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3205:
URL: https://github.com/apache/bookkeeper/pull/3205#issuecomment-1098226631
rerun failure checks
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] zymap commented on pull request #3205: Fix force GC doesn't work under forceAllowCompaction when disk is full
Posted by GitBox <gi...@apache.org>.
zymap commented on PR #3205:
URL: https://github.com/apache/bookkeeper/pull/3205#issuecomment-1098680295
But after your change, it will run the major compaction first, which may lead to the dist exhaust as well and cannot compact the entry, right?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3205: Fix force GC doesn't work under forceAllowCompaction when disk is full
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3205:
URL: https://github.com/apache/bookkeeper/pull/3205#issuecomment-1099071046
> But after your change, it will run the major compaction first, which may lead to the dist exhaust as well and cannot compact the entry, right?
@zymap In order to keep the original logic, I add two flag for REST API to trigger GC, Please take a look, thanks.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] nicoloboschi commented on a diff in pull request #3205: Fix force GC doesn't work under forceAllowCompaction when disk is full
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on code in PR #3205:
URL: https://github.com/apache/bookkeeper/pull/3205#discussion_r850367004
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/server/http/service/TriggerGCService.java:
##########
@@ -61,7 +62,16 @@ public HttpServiceResponse handle(HttpServiceRequest request) throws Exception {
HttpServiceResponse response = new HttpServiceResponse();
if (HttpServer.Method.PUT == request.getMethod()) {
- bookieServer.getBookie().getLedgerStorage().forceGC();
+ String requestBody = request.getBody();
+ if (null == requestBody) {
+ bookieServer.getBookie().getLedgerStorage().forceGC();
+ } else {
+ @SuppressWarnings("unchecked")
+ HashMap<String, Object> configMap = JsonUtil.fromJson(requestBody, HashMap.class);
Review Comment:
in general it is better to use interface here, like `java.util.Map`. just a nit
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java:
##########
@@ -332,6 +332,95 @@ public void checkpointComplete(Checkpoint checkPoint, boolean compact)
});
}
+ @Test
+ public void testForceGarbageCollectionWhenDiskIsFull() throws Exception {
+ testForceGarbageCollectionWhenDiskIsFull(true);
+ testForceGarbageCollectionWhenDiskIsFull(false);
+ }
+
+ public void testForceGarbageCollectionWhenDiskIsFull(boolean isForceCompactionAllowWhenDisableCompaction)
+ throws Exception {
+
+ restartBookies(conf -> {
+ if (isForceCompactionAllowWhenDisableCompaction) {
+ conf.setMinorCompactionInterval(0);
+ conf.setMajorCompactionInterval(0);
+ conf.setForceAllowCompaction(true);
+ conf.setMajorCompactionThreshold(0.5f);
+ conf.setMinorCompactionThreshold(0.2f);
+ } else {
+ conf.setMinorCompactionInterval(120000);
Review Comment:
can we use lower interval? this test will take several minutes to complete
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] zymap commented on a diff in pull request #3205: Fix force GC doesn't work under forceAllowCompaction when disk is full
Posted by GitBox <gi...@apache.org>.
zymap commented on code in PR #3205:
URL: https://github.com/apache/bookkeeper/pull/3205#discussion_r850947736
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java:
##########
@@ -332,6 +332,95 @@ public void checkpointComplete(Checkpoint checkPoint, boolean compact)
});
}
+ @Test
+ public void testForceGarbageCollectionWhenDiskIsFull() throws Exception {
+ testForceGarbageCollectionWhenDiskIsFull(true);
+ testForceGarbageCollectionWhenDiskIsFull(false);
+ }
+
+ public void testForceGarbageCollectionWhenDiskIsFull(boolean isForceCompactionAllowWhenDisableCompaction)
+ throws Exception {
+
+ restartBookies(conf -> {
+ if (isForceCompactionAllowWhenDisableCompaction) {
+ conf.setMinorCompactionInterval(0);
+ conf.setMajorCompactionInterval(0);
+ conf.setForceAllowCompaction(true);
+ conf.setMajorCompactionThreshold(0.5f);
+ conf.setMinorCompactionThreshold(0.2f);
+ } else {
+ conf.setMinorCompactionInterval(120000);
+ conf.setMajorCompactionInterval(240000);
+ }
+ return conf;
+ });
+
+ getGCThread().suspendMajorGC();
+ getGCThread().suspendMinorGC();
+ long majorCompactionCntBeforeGC = 0;
+ long minorCompactionCntBeforeGC = 0;
+ long majorCompactionCntAfterGC = 0;
+ long minorCompactionCntAfterGC = 0;
+
+ // disable forceMajor and forceMinor
+ majorCompactionCntBeforeGC = getGCThread().getGarbageCollectionStatus().getMajorCompactionCounter();
+ minorCompactionCntBeforeGC = getGCThread().getGarbageCollectionStatus().getMinorCompactionCounter();
+ getGCThread().triggerGC(true, true, true).get();
+ majorCompactionCntAfterGC = getGCThread().getGarbageCollectionStatus().getMajorCompactionCounter();
+ minorCompactionCntAfterGC = getGCThread().getGarbageCollectionStatus().getMinorCompactionCounter();
+ if (isForceCompactionAllowWhenDisableCompaction) {
Review Comment:
What's the difference between the if and else? Looks like they are asserting the same thing?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org