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/02/11 21:46:08 UTC
[GitHub] [bookkeeper] dlg99 opened a new pull request #3043: [ISSUE 3040] RocksDB segfaulted during CompactionTest
dlg99 opened a new pull request #3043:
URL: https://github.com/apache/bookkeeper/pull/3043
Descriptions of the changes in this PR:
### Motivation
RocksDB segfaulted during CompactionTest
### Changes
RocksDB can segfault if one tries to use it after close.
[Shutdown/compaction sequence](https://github.com/apache/bookkeeper/issues/3040#issuecomment-1036508397) can lead to such situation. The fix prevents segfault.
CompactionTests were updated at some point to use metadata cache and non-cached case is not tested.
I added the test suites for this case.
Master Issue: #3040
--
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 change in pull request #3043: [ISSUE 3040] RocksDB segfaulted during CompactionTest
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on a change in pull request #3043:
URL: https://github.com/apache/bookkeeper/pull/3043#discussion_r805079428
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
##########
@@ -400,27 +400,32 @@ public void runWithFlags(boolean force, boolean suspendMajor, boolean suspendMin
// enter major compaction
LOG.info("Enter major compaction, suspendMajor {}", suspendMajor);
majorCompacting.set(true);
- doCompactEntryLogs(majorCompactionThreshold, majorCompactionMaxTimeMillis);
- lastMajorCompactionTime = System.currentTimeMillis();
- // and also move minor compaction time
- lastMinorCompactionTime = lastMajorCompactionTime;
- gcStats.getMajorCompactionCounter().inc();
- majorCompacting.set(false);
+ try {
+ doCompactEntryLogs(majorCompactionThreshold, majorCompactionMaxTimeMillis);
+ } finally {
+ lastMajorCompactionTime = System.currentTimeMillis();
+ // and also move minor compaction time
+ lastMinorCompactionTime = lastMajorCompactionTime;
+ gcStats.getMajorCompactionCounter().inc();
+ majorCompacting.set(false);
+ }
} else if (((isForceMinorCompactionAllow && force) || (enableMinorCompaction
&& (force || curTime - lastMinorCompactionTime > minorCompactionInterval)))
&& (!suspendMinor)) {
// enter minor compaction
LOG.info("Enter minor compaction, suspendMinor {}", suspendMinor);
minorCompacting.set(true);
- doCompactEntryLogs(minorCompactionThreshold, minorCompactionMaxTimeMillis);
- lastMinorCompactionTime = System.currentTimeMillis();
- gcStats.getMinorCompactionCounter().inc();
- minorCompacting.set(false);
- }
-
- if (force && forceGarbageCollection.compareAndSet(true, false)) {
- LOG.info("{} Set forceGarbageCollection to false after force GC to make it forceGC-able again.",
- Thread.currentThread().getName());
+ try {
+ doCompactEntryLogs(minorCompactionThreshold, minorCompactionMaxTimeMillis);
+ } finally {
+ lastMinorCompactionTime = System.currentTimeMillis();
+ gcStats.getMinorCompactionCounter().inc();
+ minorCompacting.set(false);
+ }
+ if (force && forceGarbageCollection.compareAndSet(true, false)) {
Review comment:
why did you move this statement inside the above `else if` ?
--
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] dlg99 commented on a change in pull request #3043: [ISSUE 3040] RocksDB segfaulted during CompactionTest
Posted by GitBox <gi...@apache.org>.
dlg99 commented on a change in pull request #3043:
URL: https://github.com/apache/bookkeeper/pull/3043#discussion_r805082073
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
##########
@@ -400,27 +400,32 @@ public void runWithFlags(boolean force, boolean suspendMajor, boolean suspendMin
// enter major compaction
LOG.info("Enter major compaction, suspendMajor {}", suspendMajor);
majorCompacting.set(true);
- doCompactEntryLogs(majorCompactionThreshold, majorCompactionMaxTimeMillis);
- lastMajorCompactionTime = System.currentTimeMillis();
- // and also move minor compaction time
- lastMinorCompactionTime = lastMajorCompactionTime;
- gcStats.getMajorCompactionCounter().inc();
- majorCompacting.set(false);
+ try {
+ doCompactEntryLogs(majorCompactionThreshold, majorCompactionMaxTimeMillis);
+ } finally {
+ lastMajorCompactionTime = System.currentTimeMillis();
+ // and also move minor compaction time
+ lastMinorCompactionTime = lastMajorCompactionTime;
+ gcStats.getMajorCompactionCounter().inc();
+ majorCompacting.set(false);
+ }
} else if (((isForceMinorCompactionAllow && force) || (enableMinorCompaction
&& (force || curTime - lastMinorCompactionTime > minorCompactionInterval)))
&& (!suspendMinor)) {
// enter minor compaction
LOG.info("Enter minor compaction, suspendMinor {}", suspendMinor);
minorCompacting.set(true);
- doCompactEntryLogs(minorCompactionThreshold, minorCompactionMaxTimeMillis);
- lastMinorCompactionTime = System.currentTimeMillis();
- gcStats.getMinorCompactionCounter().inc();
- minorCompacting.set(false);
- }
-
- if (force && forceGarbageCollection.compareAndSet(true, false)) {
- LOG.info("{} Set forceGarbageCollection to false after force GC to make it forceGC-able again.",
- Thread.currentThread().getName());
+ try {
+ doCompactEntryLogs(minorCompactionThreshold, minorCompactionMaxTimeMillis);
+ } finally {
+ lastMinorCompactionTime = System.currentTimeMillis();
+ gcStats.getMinorCompactionCounter().inc();
+ minorCompacting.set(false);
+ }
+ if (force && forceGarbageCollection.compareAndSet(true, false)) {
Review comment:
fixed. I meant to move under finally, but under different finally.
--
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] dlg99 merged pull request #3043: [ISSUE 3040] RocksDB segfaulted during CompactionTest
Posted by GitBox <gi...@apache.org>.
dlg99 merged pull request #3043:
URL: https://github.com/apache/bookkeeper/pull/3043
--
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] dlg99 merged pull request #3043: [ISSUE 3040] RocksDB segfaulted during CompactionTest
Posted by GitBox <gi...@apache.org>.
dlg99 merged pull request #3043:
URL: https://github.com/apache/bookkeeper/pull/3043
--
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