Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch #2796

RaulGracia · 2021-09-16T09:38:09Z

Motivation

If a Bookie uses the new Bookie ID scheme explicitly (set via configuration), it should prevail over default network information to identify the Bookie and lookup for cookies in Zookeeper. Otherwise, the Bookie may fetch legacy cookies related to the same hostname but are invalid because the id differs. Essentially, this PR tries to choose between a configuration-based Bookie ID or a default network info ID when looking for cookies in Zookeeper (considering both is problematic in some cases).

Changes

The change is simple: when collecting the Bookie IDs that will be used to fetch cookies from Zookeeper (BookieImpl.possibleBookieIds()), the code now checks if there is a Bookie ID passed by configuration. If so, we just used that one for lookup cookies in Zookeeper. Otherwise, we keep the existing behavior of using network information to fetch cookies from Zookeeper.

Master Issue: #2795

Signed-off-by: Raúl <raul.gracia@emc.com>

eolivelli · 2021-09-16T13:11:15Z

This change makes sense to me.

The upgrade story for enabling BookieID is to compute the same BookieID as it is written in the bookie, like you did in Pravega, is this correct ?

We should write this somewhere in the docs (not in the scope of this PR)

In order to finalise the patch we have to add a unit test, probably a unit test about this method that you changed is enough.
A test that shows how to switch to BookieID will be also very good

Signed-off-by: Raúl <raul.gracia@emc.com>

RaulGracia · 2021-09-29T10:36:07Z

@eolivelli added a couple of tests to check the behavior of using Bookie ID in the configuration.

eolivelli

LGTM

nicoloboschi

LGTM

dlg99 · 2021-10-05T18:21:22Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java

+        assertEquals(customBookieId, server.getBookieId().toString());
+        server.shutdown();


The test expected exception, comment says to expect exception after the start.
Do you need assert and shutdown after the code that's supposed to throw?

very good point @dlg99
we should verify the Exception in a catch block and call "shutdown" in a finally block in order to perform any cleanup that is needed (and do not fall into leaks)

Thanks for the feedback. Now the test catches the expected exception and shuts down the mock server at the end of the test.

Signed-off-by: Raúl <raul.gracia@emc.com>

BP-44: USE metrics. A proposal for improving BookKeeper metrics so that operators can employ the USE method for diagnosing performance issues. Reviewers: Henry Saputra <hsaputra@apache.org>, Andrey Yegorov <None>, Enrico Olivelli <eolivelli@gmail.com> This closes #2835 from Vanlightly/BP-44-use-metrics and squashes the following commits: 8d9baab [Jack Vanlightly] Added link to USE method and listed each term of USE 5a0f67d [Jack Vanlightly] BP-44 USE metrics a9b576d [Yunze Xu] Release semaphore when addEntry accepts the same entries (#2832) 148bf22 [Yun Tang] Ensure to release cache during KeyValueStorageRocksDB#closec (#2821) 4dc4260 [gaozhangmin] Heap memory leak problem when ledger replication failed (#2794) a522fa3 [Raúl Gracia] Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 (#2816) 0465052 [Nicolò Boschi] Upgrade httpclient from 4.5.5 to 4.5.13 (#2793) 594a056 [Raúl Gracia] Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch (#2796) 354cf37 [Raúl Gracia] Upgraded dependencies with CVEs (#2792) e413c70 [Raúl Gracia] Issue 2728: Entry Log GC may get blocked when using entryLogPerLedgerEnabled option (#2779) 883231e [pradeepbn] Building bookkeeper with gradle on java11

zymap · 2021-10-25T01:59:39Z

Because it's based on an interface change(#2717 ), remove it from release 4.14.3

### Motivation In order to complete migration to Gradle we must build all the subprojects. ### Changes - Enabled `sh` integration tests with gradle, located in `tests/scripts/src/test/bash/gradle` - Added these modules to the build - `bookkeeper-http:servlet-http-server` - `metadata-drivers:etcd` - `tests:backward-compat:*` - `tests:shaded:*` - `stream:bk-grpc-name-resolver` - DL shading process is now performed (before it didn't build any jar) - Groovy tests (`tests:backward-compat:*`) now are triggered by the build/tests itself; with Maven, there is a "runner" project (`tests/integration-tests-base-groovy`); in Gradle is useless so it is skipped ### Test - Both `bin/bookkeper standalone` and `bin/bookkeper_gradle standalone` work locally - Tests are passing locally Master Issue: #2849 Reviewers: Henry Saputra <hsaputra@apache.org>, Prashant Kumar <None> This closes #2850 from nicoloboschi/fix/2849/gradle and squashes the following commits: 00b49f4 [Nicolò Boschi] Fix common_gradle.sh regex bd739fd [Nicolò Boschi] fix sh tests 43230ba [Nicolò Boschi] revert sh files. Avoid to modify maven files, create gradle versions to faciltate migration d1f95e4 [Nicolò Boschi] fix shaded deps bcab40d [Nicolò Boschi] fix build 5fd0341 [Nicolò Boschi] fix build 0082e0e [Nicolò Boschi] fix build 2c32ac1 [Nicolò Boschi] fixes 3bc0b26 [Nicolò Boschi] bookkeeper-server-shaded-tests ba89132 [Nicolò Boschi] shaded tests 6d39e33 [Nicolò Boschi] sh tests e0032bc [Nicolò Boschi] actually run arquillian groovy tests 08dcc39 [Nicolò Boschi] backwards 2361f79 [Nicolò Boschi] hierarchical-ledger-manager 8388e11 [Nicolò Boschi] current-server-old-clients 6a24344 [Nicolò Boschi] bc-non-fips 2faca01 [Nicolò Boschi] bk-grpc-name-resolver 991bc11 [Nicolò Boschi] servlet-http-server 675ef7b [Nicolò Boschi] etcd b1d5e14 [ZhangJian He] A empty implement in EtcdLedgerManagerFactory to let the project can compile (#2845) bd5c50b [shustsud] Add error handling to readLedgerMetadata in over-replicated ledger GC (#2844) 746f9f6 [Matteo Merli] Remove direct ZK access for Auditor (#2842) 4117200 [ZhangJian He] the compare should be >= instead of > (#2782) 14ef56f [Prashant Kumar] BookieId can not be cast to BookieSocketAddress (#2843) e10f3fe [ZhangJian He] Forget to close preAllocator log on shutdown (#2819) 53954ca [shustsud] Add ensemble check to over-replicated ledger GC (#2813) 919fdd3 [Prashant Kumar] Issue:2840 Create bookie shellscript for gradle (#2841) 031d168 [gaozhangmin] fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver (#2788) 3dd671c [Prashant Kumar] Migrate bookkeepr-server:test to gradle run unit tests excepts org.apache.bookkeeper.bookie. org.apache.bookkeeper.client org.apache.bookkeeper.replication org.apache.bookkeeper.tls. (#2812) f6903b8 [Jack Vanlightly] BP-44 USE metrics a4afaa4 [Matteo Merli] Eliminate direct ZK access in ScanAndCompareGarbageCollector (#2833) a9b576d [Yunze Xu] Release semaphore when addEntry accepts the same entries (#2832) 148bf22 [Yun Tang] Ensure to release cache during KeyValueStorageRocksDB#closec (#2821) 4dc4260 [gaozhangmin] Heap memory leak problem when ledger replication failed (#2794) a522fa3 [Raúl Gracia] Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 (#2816) 0465052 [Nicolò Boschi] Upgrade httpclient from 4.5.5 to 4.5.13 (#2793) 594a056 [Raúl Gracia] Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch (#2796) 354cf37 [Raúl Gracia] Upgraded dependencies with CVEs (#2792) e413c70 [Raúl Gracia] Issue 2728: Entry Log GC may get blocked when using entryLogPerLedgerEnabled option (#2779) 883231e [pradeepbn] Building bookkeeper with gradle on java11

… mismatch (apache#2796)

BP-44: USE metrics. A proposal for improving BookKeeper metrics so that operators can employ the USE method for diagnosing performance issues. Reviewers: Henry Saputra <hsaputra@apache.org>, Andrey Yegorov <None>, Enrico Olivelli <eolivelli@gmail.com> This closes apache#2835 from Vanlightly/BP-44-use-metrics and squashes the following commits: 8d9baab [Jack Vanlightly] Added link to USE method and listed each term of USE 5a0f67d [Jack Vanlightly] BP-44 USE metrics a9b576d [Yunze Xu] Release semaphore when addEntry accepts the same entries (apache#2832) 148bf22 [Yun Tang] Ensure to release cache during KeyValueStorageRocksDB#closec (apache#2821) 4dc4260 [gaozhangmin] Heap memory leak problem when ledger replication failed (apache#2794) a522fa3 [Raúl Gracia] Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 (apache#2816) 0465052 [Nicolò Boschi] Upgrade httpclient from 4.5.5 to 4.5.13 (apache#2793) 594a056 [Raúl Gracia] Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch (apache#2796) 354cf37 [Raúl Gracia] Upgraded dependencies with CVEs (apache#2792) e413c70 [Raúl Gracia] Issue 2728: Entry Log GC may get blocked when using entryLogPerLedgerEnabled option (apache#2779) 883231e [pradeepbn] Building bookkeeper with gradle on java11

### Motivation In order to complete migration to Gradle we must build all the subprojects. ### Changes - Enabled `sh` integration tests with gradle, located in `tests/scripts/src/test/bash/gradle` - Added these modules to the build - `bookkeeper-http:servlet-http-server` - `metadata-drivers:etcd` - `tests:backward-compat:*` - `tests:shaded:*` - `stream:bk-grpc-name-resolver` - DL shading process is now performed (before it didn't build any jar) - Groovy tests (`tests:backward-compat:*`) now are triggered by the build/tests itself; with Maven, there is a "runner" project (`tests/integration-tests-base-groovy`); in Gradle is useless so it is skipped ### Test - Both `bin/bookkeper standalone` and `bin/bookkeper_gradle standalone` work locally - Tests are passing locally Master Issue: apache#2849 Reviewers: Henry Saputra <hsaputra@apache.org>, Prashant Kumar <None> This closes apache#2850 from nicoloboschi/fix/2849/gradle and squashes the following commits: 00b49f4 [Nicolò Boschi] Fix common_gradle.sh regex bd739fd [Nicolò Boschi] fix sh tests 43230ba [Nicolò Boschi] revert sh files. Avoid to modify maven files, create gradle versions to faciltate migration d1f95e4 [Nicolò Boschi] fix shaded deps bcab40d [Nicolò Boschi] fix build 5fd0341 [Nicolò Boschi] fix build 0082e0e [Nicolò Boschi] fix build 2c32ac1 [Nicolò Boschi] fixes 3bc0b26 [Nicolò Boschi] bookkeeper-server-shaded-tests ba89132 [Nicolò Boschi] shaded tests 6d39e33 [Nicolò Boschi] sh tests e0032bc [Nicolò Boschi] actually run arquillian groovy tests 08dcc39 [Nicolò Boschi] backwards 2361f79 [Nicolò Boschi] hierarchical-ledger-manager 8388e11 [Nicolò Boschi] current-server-old-clients 6a24344 [Nicolò Boschi] bc-non-fips 2faca01 [Nicolò Boschi] bk-grpc-name-resolver 991bc11 [Nicolò Boschi] servlet-http-server 675ef7b [Nicolò Boschi] etcd b1d5e14 [ZhangJian He] A empty implement in EtcdLedgerManagerFactory to let the project can compile (apache#2845) bd5c50b [shustsud] Add error handling to readLedgerMetadata in over-replicated ledger GC (apache#2844) 746f9f6 [Matteo Merli] Remove direct ZK access for Auditor (apache#2842) 4117200 [ZhangJian He] the compare should be >= instead of > (apache#2782) 14ef56f [Prashant Kumar] BookieId can not be cast to BookieSocketAddress (apache#2843) e10f3fe [ZhangJian He] Forget to close preAllocator log on shutdown (apache#2819) 53954ca [shustsud] Add ensemble check to over-replicated ledger GC (apache#2813) 919fdd3 [Prashant Kumar] Issue:2840 Create bookie shellscript for gradle (apache#2841) 031d168 [gaozhangmin] fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver (apache#2788) 3dd671c [Prashant Kumar] Migrate bookkeepr-server:test to gradle run unit tests excepts org.apache.bookkeeper.bookie. org.apache.bookkeeper.client org.apache.bookkeeper.replication org.apache.bookkeeper.tls. (apache#2812) f6903b8 [Jack Vanlightly] BP-44 USE metrics a4afaa4 [Matteo Merli] Eliminate direct ZK access in ScanAndCompareGarbageCollector (apache#2833) a9b576d [Yunze Xu] Release semaphore when addEntry accepts the same entries (apache#2832) 148bf22 [Yun Tang] Ensure to release cache during KeyValueStorageRocksDB#closec (apache#2821) 4dc4260 [gaozhangmin] Heap memory leak problem when ledger replication failed (apache#2794) a522fa3 [Raúl Gracia] Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 (apache#2816) 0465052 [Nicolò Boschi] Upgrade httpclient from 4.5.5 to 4.5.13 (apache#2793) 594a056 [Raúl Gracia] Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch (apache#2796) 354cf37 [Raúl Gracia] Upgraded dependencies with CVEs (apache#2792) e413c70 [Raúl Gracia] Issue 2728: Entry Log GC may get blocked when using entryLogPerLedgerEnabled option (apache#2779) 883231e [pradeepbn] Building bookkeeper with gradle on java11

RaulGracia added 2 commits September 16, 2021 10:17

Look for cookies either using Bookie ID if provided or network info.

d09efc7

Signed-off-by: Raúl <raul.gracia@emc.com>

Fix checkstyle

3033f8c

Signed-off-by: Raúl <raul.gracia@emc.com>

SrishT mentioned this pull request Sep 21, 2021

Provide a way to make the BookieID changes backward compatible pravega/bookkeeper-operator#160

Closed

Add tests to check behavior of Bookie ID setting.

fa06f50

Signed-off-by: Raúl <raul.gracia@emc.com>

RaulGracia marked this pull request as ready for review September 29, 2021 10:35

eolivelli requested review from dlg99, fpj, merlimat, reddycharan and sijie October 5, 2021 09:09

eolivelli approved these changes Oct 5, 2021

View reviewed changes

nicoloboschi approved these changes Oct 5, 2021

View reviewed changes

dlg99 reviewed Oct 5, 2021

View reviewed changes

Improved test assertions and cleanup.

caac20c

Signed-off-by: Raúl <raul.gracia@emc.com>

dlg99 approved these changes Oct 12, 2021

View reviewed changes

eolivelli merged commit 594a056 into apache:master Oct 13, 2021

eolivelli assigned RaulGracia Oct 13, 2021

eolivelli added area/bookie area/metadata labels Oct 13, 2021

eolivelli added this to the 4.15.0 milestone Oct 13, 2021

zymap added the release/4.14.3 label Oct 15, 2021

RaulGracia mentioned this pull request Oct 15, 2021

Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch #2795

Closed

zymap removed the release/4.14.3 label Oct 25, 2021

RaulGracia mentioned this pull request Jun 3, 2022

Issue 3306: Bookie can't start after rebooting due the cookie mistmatch #3308

Merged

Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024

Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie…

41cc0e3

… mismatch (apache#2796)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch #2796

Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch #2796

Uh oh!

RaulGracia commented Sep 16, 2021

Uh oh!

eolivelli commented Sep 16, 2021

Uh oh!

RaulGracia commented Sep 29, 2021

Uh oh!

eolivelli left a comment

Uh oh!

nicoloboschi left a comment

Uh oh!

dlg99 Oct 5, 2021

Uh oh!

eolivelli Oct 6, 2021

Uh oh!

RaulGracia Oct 6, 2021

Uh oh!

zymap commented Oct 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		assertEquals(customBookieId, server.getBookieId().toString());
		server.shutdown();

Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch #2796

Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch #2796

Uh oh!

Conversation

RaulGracia commented Sep 16, 2021

Motivation

Changes

Uh oh!

eolivelli commented Sep 16, 2021

Uh oh!

RaulGracia commented Sep 29, 2021

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

nicoloboschi left a comment

Choose a reason for hiding this comment

Uh oh!

dlg99 Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

eolivelli Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

RaulGracia Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

zymap commented Oct 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants