Issue 870: ScanAndCompareGarbageCollector: harden against LM bugs #876

athanatos · 2017-12-18T22:56:00Z

The idea behind this patch is to make it more likely that more than one
LedgerManager bug would be required to erroneously believe a ledger to be
deleted.

Remove the special handling for the completely empty LM. It's not
really a common case nor is it much of an optimization even when it
happens.
Add a config variable to cause the bookie to also check the
LedgerManager.readLedgerMetadata path before actually removing a ledger.
The implementations of readLedgerMetadata and the iterator are
sufficiently different that it seems worth it to have the option of
checking both to guard somewhat against a bug in either.

(@bug W-4292747@)
(@bug W-3027938@)
Signed-off-by: Samuel Just sjust@salesforce.com
Signed-off-by: Charan Reddy Guttapalem cguttapalem@salesforce.com

Master Issue: #870

athanatos · 2017-12-18T22:56:36Z

@jvrao You might want to glance at how I handled the double check.

sijie · 2017-12-19T00:47:43Z

...keeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java

+                                latch.countDown();
+                            });
+                            latch.await();
+                            if (metaRC.get() != BKException.Code.NoBookieAvailableException) {


wrong return code here? it should be NoLedgerExistsException?

I am not sure how the below test cases work, if the return code is wrong.

Yeah, sorry. I needed to adapt that bit somewhat from our branch and evidently introduced a typo. Let me figure out why the tests worked and correct it.

Ah, it passed because there were no "normal" gc tests where either the iterator or the ledger manager weren't corrupted with that option enabled. Enabling it for testGcLedgersWithLedgersInSameLedgerRange catches this bug.

sijie · 2017-12-19T00:51:59Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java

@@ -431,4 +657,59 @@ public boolean waitForLastAddConfirmedUpdate(long ledgerId,
            return false;
        }
    }
+
+    class LedgerManagerWrapper implements LedgerManager {


I think you can use CleanupLedgerManager for this. You don't need to recreate a similar class here.

sijie · 2017-12-19T00:53:44Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java

+     * NoSuchLedgerExistsException.
+     *
+     */
+    @Test(timeout = 60000)


remove timeout value. currently we use a global timeout setting to control. If this test case doesn't require special timeout value, we can remove it here.

athanatos · 2017-12-19T18:59:55Z

@sijie Updated.

sijie

@athanatos LGTM +1

sijie · 2017-12-19T21:23:54Z

jdk9 passed, jdk8 failed with TestBenchmark (but it is unrelated, known to be a flaky test)

ArvinDevel · 2017-12-20T02:41:22Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java

+    }
+
+    /**
+     * Set whether to use transactional compaction and using a separate log for compaction or not.


the description is not correct yet

nice catch @ArvinDevel !

Thanks, will update.

reddycharan · 2017-12-20T18:32:26Z

...keeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java

                if (LOG.isDebugEnabled()) {
                    LOG.debug("Active in metadata {}, Active in bookie {}", ledgersInMetadata, subBkActiveLedgers);
                }
                for (Long bkLid : subBkActiveLedgers) {
                    if (!ledgersInMetadata.contains(bkLid)) {
+                        if (conf.getVerifyMetadataOnGC()) {


nit: why does this config variable has to be read every single time..probably a variable would be sufficient

The price of the config read seems to me to be trivial compared to the cost of querying zk. I'm inclined to keep it simple.

I'm just saying instead of reading config everytime that config value can be assigned to a final variable and use it here..anyhow that config value is not going to change just like rest of the configs

Ok, I'll switch it.

reddycharan · 2017-12-20T18:35:16Z

...keeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java

+                            });
+                            latch.await();
+                            if (metaRC.get() != BKException.Code.NoSuchLedgerExistsException) {
+                                LOG.info(


nit: warn or error?

Yeah, good point, will fix.

@bug

The idea behind this patch is to make it more likely that more than one LedgerManager bug would be required to erroneously believe a ledger to be deleted. 1) Remove the special handling for the completely empty LM. It's not really a common case nor is it much of an optimization even when it happens. 2) Add a config variable to cause the bookie to also check the LedgerManager.readLedgerMetadata path before actually removing a ledger. The implementations of readLedgerMetadata and the iterator are sufficiently different that it seems worth it to have the option of checking both to guard somewhat against a bug in either. (@bug W-4292747@) (@bug W-3027938@) Signed-off-by: Samuel Just <sjust@salesforce.com> Signed-off-by: Charan Reddy Guttapalem <cguttapalem@salesforce.com>

ArvinDevel · 2017-12-21T03:13:44Z

+1 LGTM

The idea behind this patch is to make it more likely that more than one LedgerManager bug would be required to erroneously believe a ledger to be deleted. 1) Remove the special handling for the completely empty LM. It's not really a common case nor is it much of an optimization even when it happens. 2) Add a config variable to cause the bookie to also check the LedgerManager.readLedgerMetadata path before actually removing a ledger. The implementations of readLedgerMetadata and the iterator are sufficiently different that it seems worth it to have the option of checking both to guard somewhat against a bug in either. (bug W-4292747) (bug W-3027938) Signed-off-by: Samuel Just <sjustsalesforce.com> Signed-off-by: Charan Reddy Guttapalem <cguttapalemsalesforce.com> Master Issue: apache#870 Author: Samuel Just <sjust@salesforce.com> Reviewers: Ivan Kelly <ivank@apache.org>, Arvin <None>, Sijie Guo <sijie@apache.org> This closes apache#876 from athanatos/forupstream/issue-870, closes apache#870

sijie reviewed Dec 19, 2017

View reviewed changes

athanatos force-pushed the forupstream/issue-870 branch from aea2a23 to a820ab4 Compare December 19, 2017 18:59

sijie reviewed Dec 19, 2017

View reviewed changes

sijie approved these changes Dec 19, 2017

View reviewed changes

ArvinDevel suggested changes Dec 20, 2017

View reviewed changes

athanatos force-pushed the forupstream/issue-870 branch from a820ab4 to 348cf3f Compare December 20, 2017 18:21

reddycharan reviewed Dec 20, 2017

View reviewed changes

athanatos force-pushed the forupstream/issue-870 branch from 348cf3f to 65a83f5 Compare December 20, 2017 19:13

athanatos force-pushed the forupstream/issue-870 branch from 65a83f5 to bee06be Compare December 20, 2017 19:42

ivankelly approved these changes Dec 21, 2017

View reviewed changes

sijie added this to the 4.7.0 milestone Dec 27, 2017

sijie added area/metadata release/4.7.0 type/improvement labels Dec 27, 2017

sijie assigned athanatos Dec 27, 2017

sijie closed this in 1f8b26d Dec 27, 2017

sijie mentioned this pull request Jan 11, 2018

Add sanity check in GC to prevent active ledger deletion YahooArchive/bookkeeper#15

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 870: ScanAndCompareGarbageCollector: harden against LM bugs #876

Issue 870: ScanAndCompareGarbageCollector: harden against LM bugs #876

athanatos commented Dec 18, 2017 •

edited

Loading

athanatos commented Dec 18, 2017

sijie Dec 19, 2017

sijie Dec 19, 2017

athanatos Dec 19, 2017

athanatos Dec 19, 2017

sijie Dec 19, 2017

athanatos Dec 19, 2017

sijie Dec 19, 2017

athanatos Dec 19, 2017

athanatos commented Dec 19, 2017

sijie left a comment

sijie commented Dec 19, 2017

ArvinDevel Dec 20, 2017

sijie Dec 20, 2017

athanatos Dec 20, 2017

reddycharan Dec 20, 2017

athanatos Dec 20, 2017

reddycharan Dec 20, 2017

athanatos Dec 20, 2017

reddycharan Dec 20, 2017

athanatos Dec 20, 2017

ArvinDevel commented Dec 21, 2017

Issue 870: ScanAndCompareGarbageCollector: harden against LM bugs #876

Issue 870: ScanAndCompareGarbageCollector: harden against LM bugs #876

Conversation

athanatos commented Dec 18, 2017 • edited Loading

athanatos commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athanatos commented Dec 19, 2017

sijie left a comment

Choose a reason for hiding this comment

sijie commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArvinDevel commented Dec 21, 2017

athanatos commented Dec 18, 2017 •

edited

Loading