Make BookieId work with PulsarRegistrationDriver (second take) #17922

eolivelli · 2022-10-04T07:22:41Z

Motivation

In Pulsar 2.10 the custom "BookieId" feature doesn't work anymore because in PulsarRegistrationDriver we are not implementing getBookieServiceInfo

This patch also partially resolves #17759

Modifications

Implement getBookieServiceInfo
This is a new version of #17762.

The main difference is the additional commit, in which we mimic the original code in BookKeeper
see https://github.com/apache/bookkeeper/blob/034ef8566ad037937a4d58a28f70631175744f53/bookkeeper-server/src/main/java/org/apache/bookkeeper/discover/ZKRegistrationClient.java#L183

We cannot perform reads from the MetadataStore in getBookieServiceInfo because it is very likely that
something will wait for the results of the returned CompletableFuture in a metadata store thread.
So we introduce a cache that is updated when the client scans the list of Bookies.

This works well in the original ZKRegistrationClient because the BookKeeper Client always looks for the available bookies
and this triggers the preloading of the local cache.

When we will upgrade to BK 4.15.1+ users will be able to leverage BP-41 flag to turn off BookKeeper Server address resolver
apache/bookkeeper#3356

Verifying this change

This change added tests

Documentation

doc-required
(Your PR needs to update docs and you will update later)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)

Matching PR in forked repository

PR in forked repository:eolivelli#17

eolivelli · 2022-10-04T08:31:13Z

/pulsar-bot rerun-failure-checks

eolivelli · 2022-10-04T09:00:59Z

@Jason918 @codelipenghui @nicoloboschi @mattisonchao @michaeljmarshall you reviewed the first version of the patch.
Would you mind to take a look to this new version ?

* Make BookieId work with PulsarRegistrationDriver * Switch to MetadataCache * checkstyle

…ller thread

...r-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarRegistrationClient.java

Jason918 · 2022-10-13T10:04:55Z

...r-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarRegistrationClient.java

@@ -153,4 +212,56 @@ private static Set<BookieId> convertToBookieAddresses(List<String> children) {
        }
        return newBookieAddrs;
    }
+
+    @Override
+    public CompletableFuture<Versioned<BookieServiceInfo>> getBookieServiceInfo(BookieId bookieId) {


I am trying to get more sense from this issue, but I don't see this method in the stack of #17913. Call you point out the call path from metadata main thread?

here (copied from #17913) we are waiting on the CompletableFuture returned by this method.

but without this PR the CF is expected to be completed by the main metadata store thread, that is already running the caller code (BookkeeperSchemaStorage.lambda$openLedger)

t java.util.concurrent.CompletableFuture.get(java.base@11.0.15.0.1/CompletableFuture.java:1998) at org.apache.bookkeeper.common.concurrent.FutureUtils.result(FutureUtils.java:72) at org.apache.bookkeeper.common.concurrent.FutureUtils.result(FutureUtils.java:61) at org.apache.bookkeeper.client.DefaultBookieAddressResolver.resolve(DefaultBookieAddressResolver.java:43) at org.apache.bookkeeper.proto.PerChannelBookieClient.connect(PerChannelBookieClient.java:532) at org.apache.bookkeeper.proto.PerChannelBookieClient.connectIfNeededAndDoOp(PerChannelBookieClient.java:658) at org.apache.bookkeeper.proto.DefaultPerChannelBookieClientPool.initialize(DefaultPerChannelBookieClientPool.java:92) at org.apache.bookkeeper.proto.BookieClientImpl.lookupClient(BookieClientImpl.java:217)

it is really complicated to see because there are so many indirections.
But the problem is here in the end.
We had the very same problem in BK when initially I implemented getBookieServiceInfo in the BK core code base (you can take a look there)

I see, getBookieServiceInfo is called in a blocking way in DefaultBookieAddressResolver#resolve.

https://github.com/apache/bookkeeper/blob/c31dff9ff8dc3ec7454a62d71a9fff2af8505ee9/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/DefaultBookieAddressResolver.java#L43

Jason918 · 2022-10-13T10:12:58Z

...r-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarRegistrationClient.java

+
+    public CompletableFuture<Versioned<BookieServiceInfo>> readBookieServiceInfoAsync(BookieId bookieId) {
+        String asWritable = bookieRegistrationPath + "/" + bookieId;
+        return bookieServiceInfoMetadataCache.get(asWritable)


You refer to this get could be a blocking call.
My best guess is when the key is loading for the first time.
But the key call path should be like :

org.apache.pulsar.metadata.cache.impl.MetadataCacheImpl#get

com.github.benmanes.caffeine.cache.AsyncLoadingCache#get

org.apache.pulsar.metadata.cache.impl.MetadataCacheImpl#readValueFromStore

and all of these should not contains any blocking operations.

AsyncLoadingCache#get

this is nasty, I know, because even if it seems "async" the "completion" of the CompletableFuture is executed on the main metadata store thread.

In another operation that runs on the same thread you cannot wait for the completion of a CompletableFuture that wills be only completed on the same thread.

I see, the THREAD (main-EventThread) is not blocked at this get method actually. The get method returns a CF without blocking, but the completion of the CF depends on the THREAD, and the THREAD is blocked at the completions of the CF. Thus the deadlock.

Jason918 · 2022-10-15T04:23:29Z

...r-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarRegistrationClient.java

+
+    final CompletableFuture<Versioned<BookieServiceInfo>> readBookieInfoAsReadonlyBookie(BookieId bookieId) {
+        String asReadonly = bookieReadonlyRegistrationPath + "/" + bookieId;
+        return bookieServiceInfoMetadataCache.get(asReadonly)


Is it still necessary to get data from bookieServiceInfoMetadataCache? It seems we already implemented a cache with bookieServiceInfoCache, and we can read with store directly.

Correct. I did it this way in order to leverage the built in support for SerDe of MetadataCache.
The overhead is not big and it is more elegant.
I can use raw reads and use the SerDe explicitly if you feel strong

Correct. I did it this way in order to leverage the built in support for SerDe of MetadataCache. The overhead is not big and it is more elegant.

This make sense for me.

Jason918

Great work!

mattisonchao · 2022-11-03T11:15:15Z

In Pulsar 2.10 the custom "BookieId" feature doesn't work anymore because in PulsarRegistrationDriver we are not implementing getBookieServiceInfo

I think we have to cherry-pick this PR to branch-2.10 and 2.11, and this PR can also fix the ZK thread-blocking problem.
/cc @eolivelli

eolivelli · 2022-11-03T11:56:15Z

Yes we can do it, to port it to branch 2.10 but there is no hurry.
I don't think that Pulsar users are using the bookieId feature even on 2.8. Because it is not supported by the Helm Chart.
We would have to port this only of someone is on 2.8 and it uses BookieID.

So I am not against porting it but I don't think it is worth, especially because it may introduce some little overhead and branch 2.10 is very stable

mattisonchao · 2022-11-03T12:14:23Z

Yes, I see. thanks! @eolivelli

* Make BookieId work with PulsarRegistrationDriver (#17762) * Make BookieId work with PulsarRegistrationDriver * Switch to MetadataCache * checkstyle * Do not execute lookup on MetadataCache in the getBookieServiceInfo caller thread (cherry picked from commit 09f5eeb)

…e#17922) * Make BookieId work with PulsarRegistrationDriver (apache#17762) * Make BookieId work with PulsarRegistrationDriver * Switch to MetadataCache * checkstyle * Do not execute lookup on MetadataCache in the getBookieServiceInfo caller thread (cherry picked from commit 09f5eeb) (cherry picked from commit e3eb026)

* Make BookieId work with PulsarRegistrationDriver (#17762) * Make BookieId work with PulsarRegistrationDriver * Switch to MetadataCache * checkstyle * Do not execute lookup on MetadataCache in the getBookieServiceInfo caller thread

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 4, 2022

eolivelli self-assigned this Oct 4, 2022

eolivelli added the ready-to-test label Oct 4, 2022

eolivelli added this to the 2.12.0 milestone Oct 4, 2022

eolivelli requested review from codelipenghui, Jason918, michaeljmarshall, nicoloboschi and mattisonchao October 4, 2022 09:10

eolivelli and others added 2 commits October 5, 2022 08:43

Make BookieId work with PulsarRegistrationDriver (apache#17762)

24eef75

* Make BookieId work with PulsarRegistrationDriver * Switch to MetadataCache * checkstyle

Do not execute lookup on MetadataCache in the getBookieServiceInfo ca…

14701af

…ller thread

eolivelli force-pushed the impl/bookieid-again branch from 5188f16 to 14701af Compare October 5, 2022 06:43

mattisonchao reviewed Oct 8, 2022

View reviewed changes

...r-metadata/src/main/java/org/apache/pulsar/metadata/bookkeeper/PulsarRegistrationClient.java Show resolved Hide resolved

Jason918 reviewed Oct 13, 2022

View reviewed changes

Jason918 reviewed Oct 15, 2022

View reviewed changes

Jason918 approved these changes Oct 15, 2022

View reviewed changes

eolivelli merged commit 09f5eeb into apache:master Oct 15, 2022

eolivelli deleted the impl/bookieid-again branch October 15, 2022 16:24

mattisonchao added release/2.10.3 release/2.11.1 labels Nov 3, 2022

horizonzy mentioned this pull request Nov 30, 2022

[Bug] The AutoRecovery didn't aware rack info after restart. #18671

Closed

2 tasks

Nicklee007 mentioned this pull request Dec 7, 2022

[Bug] BookieRackAffinityMapping is not compatible with custom BookieId #18673

Closed

2 tasks

yangl mentioned this pull request Dec 9, 2022

[Bug] cannot get the right rack info since the 2.10 version #18834

Closed

2 tasks

liangyepianzhou added the cherry-picked/branch-2.10 label Dec 13, 2022

Technoboy- added the cherry-picked/branch-2.11 label Feb 8, 2023

yapxue mentioned this pull request Apr 27, 2023

[Bug] broker cannot connect to bookie suddenly and after restarting this issue persists #20186

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make BookieId work with PulsarRegistrationDriver (second take) #17922

Make BookieId work with PulsarRegistrationDriver (second take) #17922

eolivelli commented Oct 4, 2022

eolivelli commented Oct 4, 2022

eolivelli commented Oct 4, 2022

Jason918 Oct 13, 2022

eolivelli Oct 13, 2022

Jason918 Oct 15, 2022

Jason918 Oct 13, 2022

eolivelli Oct 13, 2022

Jason918 Oct 15, 2022

Jason918 Oct 15, 2022

eolivelli Oct 15, 2022

Jason918 Oct 15, 2022

Jason918 left a comment

mattisonchao commented Nov 3, 2022

eolivelli commented Nov 3, 2022 •

edited

mattisonchao commented Nov 3, 2022

Make BookieId work with PulsarRegistrationDriver (second take) #17922

Make BookieId work with PulsarRegistrationDriver (second take) #17922

Conversation

eolivelli commented Oct 4, 2022

Motivation

Modifications

Verifying this change

Documentation

Matching PR in forked repository

eolivelli commented Oct 4, 2022

eolivelli commented Oct 4, 2022

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

Jason918 left a comment

Choose a reason for hiding this comment

mattisonchao commented Nov 3, 2022

eolivelli commented Nov 3, 2022 • edited

mattisonchao commented Nov 3, 2022

eolivelli commented Nov 3, 2022 •

edited