From af67370775e688e65919f1294fb96f0c1be108d9 Mon Sep 17 00:00:00 2001 From: Andrey Yegorov Date: Fri, 8 Jul 2016 16:00:18 -0700 Subject: [PATCH 1/6] BOOKKEEPER-938 ledger digestType autodetection on open (via configurable parameter) --- .../bookkeeper/client/LedgerOpenOp.java | 29 ++- .../bookkeeper/conf/ClientConfiguration.java | 24 +++ ...eWriteLedgersWithDifferentDigestsTest.java | 201 ++++++++++++++++++ 3 files changed, 248 insertions(+), 6 deletions(-) create mode 100644 bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieWriteLedgersWithDifferentDigestsTest.java diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java index cc978664aa8..954bd97313f 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java @@ -48,11 +48,13 @@ class LedgerOpenOp implements GenericCallback { final Object ctx; LedgerHandle lh; final byte[] passwd; - final DigestType digestType; boolean doRecovery = true; boolean administrativeOpen = false; long startTime; OpStatsLogger openOpLogger; + + final DigestType suggestedDigestType; + final boolean enableDigestAutodetection; /** * Constructor. @@ -64,6 +66,7 @@ class LedgerOpenOp implements GenericCallback { * @param cb * @param ctx */ + // needed strictly for cases when BK still has ledgers created by versions < 4.2 public LedgerOpenOp(BookKeeper bk, long ledgerId, DigestType digestType, byte[] passwd, OpenCallback cb, Object ctx) { this.bk = bk; @@ -71,9 +74,21 @@ public LedgerOpenOp(BookKeeper bk, long ledgerId, DigestType digestType, byte[] this.passwd = passwd; this.cb = cb; this.ctx = ctx; - this.digestType = digestType; + this.enableDigestAutodetection = bk.conf.getEnableDigestTypeAutodetection(); + this.suggestedDigestType = digestType; } + public LedgerOpenOp(BookKeeper bk, long ledgerId, byte[] passwd, + OpenCallback cb, Object ctx) { + this.bk = bk; + this.ledgerId = ledgerId; + this.passwd = passwd; + this.cb = cb; + this.ctx = ctx; + this.enableDigestAutodetection = true; + this.suggestedDigestType = bk.conf.getBookieRecoveryDigestType(); + } + public LedgerOpenOp(BookKeeper bk, long ledgerId, OpenCallback cb, Object ctx) { this.bk = bk; this.ledgerId = ledgerId; @@ -81,8 +96,9 @@ public LedgerOpenOp(BookKeeper bk, long ledgerId, OpenCallback cb, Object ctx) { this.ctx = ctx; this.passwd = bk.getConf().getBookieRecoveryPasswd(); - this.digestType = bk.getConf().getBookieRecoveryDigestType(); this.administrativeOpen = true; + this.enableDigestAutodetection = false; + this.suggestedDigestType = bk.conf.getBookieRecoveryDigestType(); } /** @@ -119,8 +135,10 @@ public void operationComplete(int rc, LedgerMetadata metadata) { } final byte[] passwd; - final DigestType digestType; - + DigestType digestType = enableDigestAutodetection + ? metadata.getDigestType() + : suggestedDigestType; + /* For an administrative open, the default passwords * are read from the configuration, but if the metadata * already contains passwords, use these instead. */ @@ -129,7 +147,6 @@ public void operationComplete(int rc, LedgerMetadata metadata) { digestType = metadata.getDigestType(); } else { passwd = this.passwd; - digestType = this.digestType; if (metadata.hasPassword()) { if (!Arrays.equals(passwd, metadata.getPassword())) { diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java index b8d738b04ba..77147f3c824 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java @@ -43,6 +43,8 @@ public class ClientConfiguration extends AbstractConfiguration { // Digest Type protected final static String DIGEST_TYPE = "digestType"; + protected final static String ENABLE_DIGEST_TYPE_AUTODETECTION = "enableDigestTypeAutodetection"; + // Passwd protected final static String PASSWD = "passwd"; @@ -131,6 +133,28 @@ public ClientConfiguration setThrottleValue(int throttle) { return this; } + /** + * Whether to enable autodetection of digest type. + * Ignores provided digestType, if enabled and uses one from ledger metadata instead. + * Incompatible with ledger created by bookie versions < 4.2 + * + * @return flag to enable/disable autodetection of digest type. + */ + public boolean getEnableDigestTypeAutodetection() { + return getBoolean(ENABLE_DIGEST_TYPE_AUTODETECTION, false); + } + + /** + * Whether to enable autodetection of digest type. + * Ignores provided digestType, if enabled and uses one from ledger metadata instead. + * Incompatible with ledger created by bookie versions < 4.2 + * + * @return client configuration. + */ public ClientConfiguration setEnableDigestTypeAutodetection(boolean enable) { + this.setProperty(ENABLE_DIGEST_TYPE_AUTODETECTION, enable); + return this; + } + /** * Get digest type used in bookkeeper admin * diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieWriteLedgersWithDifferentDigestsTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieWriteLedgersWithDifferentDigestsTest.java new file mode 100644 index 00000000000..9bdb80e9ff5 --- /dev/null +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieWriteLedgersWithDifferentDigestsTest.java @@ -0,0 +1,201 @@ +/** + * + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * + */ +package org.apache.bookkeeper.client; + +import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.Enumeration; +import java.util.Random; + +import org.apache.bookkeeper.client.AsyncCallback.AddCallback; +import org.apache.bookkeeper.client.BookKeeper.DigestType; +import org.apache.bookkeeper.test.MultiLedgerManagerMultiDigestTestCase; +import org.junit.Before; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.junit.Assert.*; + +/** + * Verify reads from ledgers with different digest types. + * This can happen as result of clients using different settings + * yet reading each other data or configuration change roll out. + */ +public class BookieWriteLedgersWithDifferentDigestsTest extends + MultiLedgerManagerMultiDigestTestCase implements AddCallback { + + private final static Logger LOG = LoggerFactory + .getLogger(BookieWriteLedgersWithDifferentDigestsTest.class); + + byte[] ledgerPassword = "aaa".getBytes(); + LedgerHandle lh, lh2; + Enumeration ls; + + // test related variables + final int numEntriesToWrite = 20; + int maxInt = Integer.MAX_VALUE; + Random rng; + ArrayList entries1; // generated entries + ArrayList entries2; // generated entries + + DigestType digestType; + + private static class SyncObj { + volatile int counter; + volatile int rc; + + public SyncObj() { + counter = 0; + } + } + + @Override + @Before + public void setUp() throws Exception { + super.setUp(); + rng = new Random(System.currentTimeMillis()); // Initialize the Random + // Number Generator + entries1 = new ArrayList(); // initialize the entries list + entries2 = new ArrayList(); // initialize the entries list + } + + public BookieWriteLedgersWithDifferentDigestsTest(String ledgerManagerFactory, + DigestType digestType) { + super(3); + this.digestType = digestType; + // set ledger manager + baseConf.setLedgerManagerFactoryClassName(ledgerManagerFactory); + baseClientConf.setLedgerManagerFactoryClassName(ledgerManagerFactory); + } + + @Test(timeout = 60000) + public void testLedgersWithDifferentDigestTypesNoAutodetection() throws Exception { + bkc.conf.setEnableDigestTypeAutodetection(false); + // Create ledgers + lh = bkc.createLedgerAdv(3, 2, 2, DigestType.MAC, ledgerPassword); + + final long id = lh.ledgerId; + + LOG.info("Ledger ID-1: " + lh.getId()); + SyncObj syncObj1 = new SyncObj(); + for (int i = numEntriesToWrite - 1; i >= 0; i--) { + ByteBuffer entry = ByteBuffer.allocate(4); + entry.putInt(rng.nextInt(maxInt)); + entry.position(0); + entries1.add(0, entry.array()); + lh.asyncAddEntry(i, entry.array(), 0, entry.capacity(), this, syncObj1); + } + + // Wait for all entries to be acknowledged + waitForEntriesAddition(syncObj1, numEntriesToWrite); + + // Reads here work ok because ledger uses digest type set during create + readEntries(lh, entries1); + lh.close(); + + try { + bkc.openLedgerNoRecovery(id, DigestType.CRC32, ledgerPassword).close(); + fail("digest mismatch error is expected"); + } catch (BKException bke) { + // expected + } + } + + @Test(timeout = 60000) + public void testLedgersWithDifferentDigestTypesWithAutodetection() throws Exception { + bkc.conf.setEnableDigestTypeAutodetection(true); + // Create ledgers + lh = bkc.createLedgerAdv(3, 2, 2, DigestType.MAC, ledgerPassword); + lh2 = bkc.createLedgerAdv(3, 2, 2, DigestType.CRC32, ledgerPassword); + + final long id = lh.ledgerId; + final long id2 = lh.ledgerId; + + LOG.info("Ledger ID-1: " + lh.getId()); + LOG.info("Ledger ID-2: " + lh2.getId()); + SyncObj syncObj1 = new SyncObj(); + SyncObj syncObj2 = new SyncObj(); + for (int i = numEntriesToWrite - 1; i >= 0; i--) { + ByteBuffer entry = ByteBuffer.allocate(4); + entry.putInt(rng.nextInt(maxInt)); + entry.position(0); + entries1.add(0, entry.array()); + entries2.add(0, entry.array()); + lh.asyncAddEntry(i, entry.array(), 0, entry.capacity(), this, syncObj1); + lh2.asyncAddEntry(i, entry.array(), 0, entry.capacity(), this, syncObj2); + } + + // Wait for all entries to be acknowledged + waitForEntriesAddition(syncObj1, numEntriesToWrite); + waitForEntriesAddition(syncObj2, numEntriesToWrite); + + // Reads here work ok because ledger uses digest type set during create + readEntries(lh, entries1); + readEntries(lh2, entries2); + lh.close(); + lh2.close(); + + // open here would fail if provided digest type is used + // it passes because ledger just uses digest type from its metadata/autodetects it + lh = bkc.openLedgerNoRecovery(id, DigestType.CRC32, ledgerPassword); + lh2 = bkc.openLedgerNoRecovery(id2, DigestType.MAC, ledgerPassword); + readEntries(lh, entries1); + readEntries(lh2, entries2); + lh.close(); + lh2.close(); + } + + private void waitForEntriesAddition(SyncObj syncObj, int numEntriesToWrite) throws InterruptedException { + synchronized (syncObj) { + while (syncObj.counter < numEntriesToWrite) { + syncObj.wait(); + } + assertEquals(BKException.Code.OK, syncObj.rc); + } + } + + private void readEntries(LedgerHandle lh, ArrayList entries) throws InterruptedException, BKException { + ls = lh.readEntries(0, numEntriesToWrite - 1); + int index = 0; + while (ls.hasMoreElements()) { + ByteBuffer origbb = ByteBuffer.wrap(entries.get(index++)); + Integer origEntry = origbb.getInt(); + ByteBuffer result = ByteBuffer.wrap(ls.nextElement().getEntry()); + LOG.debug("Length of result: " + result.capacity()); + LOG.debug("Original entry: " + origEntry); + Integer retrEntry = result.getInt(); + LOG.debug("Retrieved entry: " + retrEntry); + assertTrue("Checking entry " + index + " for equality", origEntry + .equals(retrEntry)); + } + } + + @Override + public void addComplete(int rc, LedgerHandle lh, long entryId, Object ctx) { + SyncObj x = (SyncObj) ctx; + synchronized (x) { + x.rc = rc; + x.counter++; + x.notify(); + } + } +} From b6ebba39fd2ec1a5d6cd0ad1184a7d6db62b9c44 Mon Sep 17 00:00:00 2001 From: Andrey Yegorov Date: Fri, 8 Jul 2016 16:07:36 -0700 Subject: [PATCH 2/6] BOOKKEEPER-938 autoformat managed to sneak tabs instead of spaces --- .../main/java/org/apache/bookkeeper/client/LedgerOpenOp.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java index 954bd97313f..b354c52b085 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java @@ -136,8 +136,8 @@ public void operationComplete(int rc, LedgerMetadata metadata) { final byte[] passwd; DigestType digestType = enableDigestAutodetection - ? metadata.getDigestType() - : suggestedDigestType; + ? metadata.getDigestType() + : suggestedDigestType; /* For an administrative open, the default passwords * are read from the configuration, but if the metadata From 00ad28cf8def77184542b5397be03071614b886c Mon Sep 17 00:00:00 2001 From: Andrey Yegorov Date: Mon, 11 Jul 2016 09:41:32 -0700 Subject: [PATCH 3/6] BOOKKEEPER-938 code review feedback: remove unused constructor --- .../org/apache/bookkeeper/client/LedgerOpenOp.java | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java index b354c52b085..6cf2721c88b 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java @@ -61,12 +61,11 @@ class LedgerOpenOp implements GenericCallback { * * @param bk * @param ledgerId - * @param digestType + * @param digestType. Ignored if conf.getEnableDigestTypeAutodetection() is true * @param passwd * @param cb * @param ctx */ - // needed strictly for cases when BK still has ledgers created by versions < 4.2 public LedgerOpenOp(BookKeeper bk, long ledgerId, DigestType digestType, byte[] passwd, OpenCallback cb, Object ctx) { this.bk = bk; @@ -78,17 +77,6 @@ public LedgerOpenOp(BookKeeper bk, long ledgerId, DigestType digestType, byte[] this.suggestedDigestType = digestType; } - public LedgerOpenOp(BookKeeper bk, long ledgerId, byte[] passwd, - OpenCallback cb, Object ctx) { - this.bk = bk; - this.ledgerId = ledgerId; - this.passwd = passwd; - this.cb = cb; - this.ctx = ctx; - this.enableDigestAutodetection = true; - this.suggestedDigestType = bk.conf.getBookieRecoveryDigestType(); - } - public LedgerOpenOp(BookKeeper bk, long ledgerId, OpenCallback cb, Object ctx) { this.bk = bk; this.ledgerId = ledgerId; From a9e7c5ce525bddf61368f7abc675d34c2bb0be58 Mon Sep 17 00:00:00 2001 From: Andrey Yegorov Date: Tue, 19 Jul 2016 11:52:00 +0300 Subject: [PATCH 4/6] BOOKKEEPER-938 spelling updated according to code review comments --- .../java/org/apache/bookkeeper/conf/ClientConfiguration.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java index 77147f3c824..6e6af5112a4 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java @@ -134,7 +134,7 @@ public ClientConfiguration setThrottleValue(int throttle) { } /** - * Whether to enable autodetection of digest type. + * Get autodetection of digest type. * Ignores provided digestType, if enabled and uses one from ledger metadata instead. * Incompatible with ledger created by bookie versions < 4.2 * @@ -145,7 +145,7 @@ public boolean getEnableDigestTypeAutodetection() { } /** - * Whether to enable autodetection of digest type. + * Enable autodetection of digest type. * Ignores provided digestType, if enabled and uses one from ledger metadata instead. * Incompatible with ledger created by bookie versions < 4.2 * From ee6832a404374bdb23492e6f9e61330a09a228e9 Mon Sep 17 00:00:00 2001 From: Andrey Yegorov Date: Thu, 28 Jul 2016 12:03:17 +0300 Subject: [PATCH 5/6] BOOKKEEPER-938 formatting fix / added line break --- .../java/org/apache/bookkeeper/conf/ClientConfiguration.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java index 6e6af5112a4..57c3790b3e6 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java @@ -150,7 +150,8 @@ public boolean getEnableDigestTypeAutodetection() { * Incompatible with ledger created by bookie versions < 4.2 * * @return client configuration. - */ public ClientConfiguration setEnableDigestTypeAutodetection(boolean enable) { + */ + public ClientConfiguration setEnableDigestTypeAutodetection(boolean enable) { this.setProperty(ENABLE_DIGEST_TYPE_AUTODETECTION, enable); return this; } From 0fb93bc796e46a33a3415c15b8ce2d7aadcb8e38 Mon Sep 17 00:00:00 2001 From: Andrey Yegorov Date: Thu, 28 Jul 2016 12:03:51 +0300 Subject: [PATCH 6/6] BOOKKEEPER-938 corrected ledger handle id in the test --- .../client/BookieWriteLedgersWithDifferentDigestsTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieWriteLedgersWithDifferentDigestsTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieWriteLedgersWithDifferentDigestsTest.java index 9bdb80e9ff5..3b28db06c06 100644 --- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieWriteLedgersWithDifferentDigestsTest.java +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieWriteLedgersWithDifferentDigestsTest.java @@ -128,7 +128,7 @@ public void testLedgersWithDifferentDigestTypesWithAutodetection() throws Except lh2 = bkc.createLedgerAdv(3, 2, 2, DigestType.CRC32, ledgerPassword); final long id = lh.ledgerId; - final long id2 = lh.ledgerId; + final long id2 = lh2.ledgerId; LOG.info("Ledger ID-1: " + lh.getId()); LOG.info("Ledger ID-2: " + lh2.getId());