[Issue 5935] Support multi pulsar clusters to use the same bk cluster #5985

murong00 · 2020-01-03T11:22:27Z

Motivation

Support multi pulsar clusters to use the specified bookkeeper cluster by pointing BookKeeper Client to the zookeeper connection string of bookkeeper cluster.

Modifications

Add a config bookkeeperServiceUri to discover bookkeeper cluster metadata store.
Use metadata service uri to init bookkeeper client.

tuteng · 2020-01-08T13:54:00Z

run integration tests
run java8 tests

sijie · 2020-01-18T21:32:09Z

pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java

        ServerConfiguration bkConf = new ServerConfiguration();
        bkConf.setZkServers(arguments.zookeeper);
        bkConf.setZkTimeout(arguments.zkSessionTimeoutMillis);
-        if (localZk.exists("/ledgers", false) == null // only format if /ledgers doesn't exist


why do we remove this logic?

just a mistake, modified.

sijie · 2020-01-18T21:34:06Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

+        doc = "BookKeeper ledgers storage connection string when using a separated BookKeeper cluster."
+            + " If not set it will use local zookeeper quorum of Pulsar cluster"
+    )
+    private String bookkeeperLedgersStore;


in bookkeeper we deprecated using two separate settings. we are encouraging people to use metadata service uri to connect to a zookeeper cluster. we should try to adopt it in Pulsar. Thus if we are adding the support for a separate bookkeeper cluster, let's try to use metadata service uri.

done, thanks.

murong00 · 2020-01-19T12:41:36Z

@sijie please help to review this.

murong00 · 2020-02-17T09:01:03Z

@sijie @jiazhai I have addressed sijie's comment, please help to review again, thanks.

murong00 · 2020-02-22T10:11:04Z

@sijie @jiazhai @tuteng How this issue going, can we finish it in milestone 2.5.1 or 2.6.0?

sijie · 2020-02-16T06:22:38Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/BookKeeperClientFactoryImpl.java

+        if (StringUtils.isNotBlank(conf.getBookkeeperServiceUri())) {
+            bkConf.setMetadataServiceUri(conf.getBookkeeperServiceUri());
+        } else {
+            PulsarService pulsar = new PulsarService(conf);


I don't think we should try to construct PulsarService. because the constructor also does a lot of unrelated things.

I would suggest adding a util function to generate metadata service url from the configuration.

sijie · 2020-02-24T20:21:20Z

@murong00 I thought I reviewed it already. but it turned out that I forgot to click submit button. Here is the review comment. PTAL

murong00 · 2020-02-25T04:15:25Z

@sijie Thanks for the review, PTAL.

jiazhai · 2020-04-02T15:15:16Z

pulsar-common/src/main/java/org/apache/pulsar/common/conf/InternalConfigurationData.java

@@ -28,19 +28,19 @@

    private String zookeeperServers;
    private String configurationStoreServers;
-    private String ledgersRootPath;
+    private String metadataServiceUri;


It seems ledgersRootPath is much more direct than metadataServiceUri. metadata may lead developer think of "zookeeper". How about change this back？

@jiazhai In fact, metadataServiceUri here is a uri to connect to a zookeeper cluster, which is used to find bookies. I would prefer to keep it since ledgersRootPath seems more like a path and is using together with zookeeper. As sijie's comment above, we are trying to use a metadata service uri, so how about bkMetadataServiceUri?

jiazhai · 2020-04-02T15:24:37Z

conf/broker.conf

@@ -461,6 +461,10 @@ httpMaxRequestSize=-1

 ### --- BookKeeper Client --- ###

+# Metadata service uri that bookkeeper is used for loading corresponding metadata driver
+# and resolving its metadata service location.
+bookkeeperServiceUri=


how about bkLedgerRootPath? This seems be more directly.
bookkeeperServiceUri may be easy confused with the bookie service url(e.g. "localhost:3181").

bkMetadataServiceUri seems better?

I will add an example here to make it more clear.

jiazhai · 2020-04-02T15:26:20Z

@murong00 Thanks for the work. left some comments related to the var names. overall lgtm. Would you please take a look, and rebase this PR with the latest master code?

murong00 · 2020-04-03T07:28:40Z

@jiazhai Thanks for the review, please help to take a look again.

murong00 · 2020-04-09T04:25:19Z

/pulsarbot run-failure-checks

Rebuild some logic Rebuild some logic Added missing changes

Fix a unit test

Fix checkstyle

murong00 · 2020-04-14T02:40:18Z

/pulsarbot run-failure-checks

jiazhai

+1.

murong00 · 2020-04-14T04:27:59Z

@sijie Please help to take a look about this again.

sijie · 2020-04-16T21:38:12Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

+        ClientConfiguration bkConf = new ClientConfiguration();
+        // init bookkeeper metadata service uri
+        String metadataServiceUri = null;
+        try {


I don't think we should add this logic here. We should reply on the logic that bookkeeper already provides. You can do this by using the following code snippet.

ClientConfiguration bkConf = new ClientConfiguration(); bkConf.setZkServers(config.getZooKeeperServers()); return bkConf.getMetadataServiceUri();

sijie · 2020-04-16T21:40:04Z

pulsar-common/src/main/java/org/apache/pulsar/common/conf/InternalConfigurationData.java

@@ -28,19 +28,19 @@

    private String zookeeperServers;
    private String configurationStoreServers;
-    private String ledgersRootPath;


for backward compatibility, you shouldn't change the field directly.

You can add a new field bookkeeperMetadataServiceUri and deprecate the old field ledgersRootPath .

Thanks for pointing this out, done.

murong00 · 2020-04-17T02:07:37Z

@sijie Thanks for the advise and review, please help to take a look.

murong00 · 2020-04-17T05:36:17Z

/pulsarbot run-failure-checks

murong00 · 2020-04-20T06:00:14Z

@sijie Could you please help to take a look when you are available? Thanks.

…apache#5985) ### Motivation Fixes apache#5935 Support multi pulsar clusters to use the specified bookkeeper cluster by pointing `BookKeeper Client` to the zookeeper connection string of bookkeeper cluster. ### Modifications - Add a config `bookkeeperServiceUri` to discover bookkeeper cluster metadata store. - Use metadata service uri to init bookkeeper client.

murong00 requested review from sijie, codelipenghui, rdhabalia and jiazhai January 6, 2020 09:42

sijie assigned murong00 Jan 13, 2020

sijie added the area/cli label Jan 13, 2020

sijie mentioned this pull request Jan 14, 2020

Allow ManagedLedger to configure using an existing bookkeeper cluster #4140

Closed

murong00 requested a review from merlimat January 17, 2020 12:04

sijie requested changes Jan 18, 2020

View reviewed changes

sijie requested changes Feb 24, 2020

View reviewed changes

sijie added this to the 2.6.0 milestone Feb 24, 2020

murong00 force-pushed the branch-5935 branch from f9acd25 to 786b310 Compare February 25, 2020 04:06

jiazhai reviewed Apr 2, 2020

View reviewed changes

murong00 force-pushed the branch-5935 branch from 7de91fa to e2d7c4b Compare April 3, 2020 07:11

murong00 force-pushed the branch-5935 branch from bbae7d3 to 4764ebf Compare April 9, 2020 02:02

murong00 and others added 4 commits April 14, 2020 09:19

Support multi pulsar clusters to use the same bk cluster.

e4e87bd

Rebuild some logic Rebuild some logic Added missing changes

Use metadata service uri to init bookkeeper client

c5062f9

Add a util function to generate metadata service url

0673e7d

Fix a unit test

Rename var names and add a test

b348adf

Fix checkstyle

murong00 force-pushed the branch-5935 branch from 4764ebf to b348adf Compare April 14, 2020 01:20

jiazhai approved these changes Apr 14, 2020

View reviewed changes

sijie requested changes Apr 16, 2020

View reviewed changes

Deprecated ledgersRootPath and use the logic of bkConf

e8e8748

sijie approved these changes Apr 20, 2020

View reviewed changes

sijie merged commit 188d896 into apache:master Apr 21, 2020

sijie mentioned this pull request May 20, 2020

[discussion] Pulsar release 2.6.0 #5819

Closed

jerrypeng mentioned this pull request Jul 14, 2020

Fix: function BC issue introduced in 2.6 #7528

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue 5935] Support multi pulsar clusters to use the same bk cluster #5985

[Issue 5935] Support multi pulsar clusters to use the same bk cluster #5985

murong00 commented Jan 3, 2020 •

edited

tuteng commented Jan 8, 2020

sijie Jan 18, 2020

murong00 Jan 19, 2020

sijie Jan 18, 2020

murong00 Jan 19, 2020

murong00 commented Jan 19, 2020

murong00 commented Feb 17, 2020

murong00 commented Feb 22, 2020

sijie Feb 16, 2020

murong00 Feb 25, 2020

sijie commented Feb 24, 2020

murong00 commented Feb 25, 2020 •

edited

jiazhai Apr 2, 2020

murong00 Apr 3, 2020

jiazhai Apr 2, 2020

murong00 Apr 3, 2020

murong00 Apr 3, 2020

jiazhai commented Apr 2, 2020

murong00 commented Apr 3, 2020

murong00 commented Apr 9, 2020

murong00 commented Apr 14, 2020

jiazhai left a comment

murong00 commented Apr 14, 2020

sijie Apr 16, 2020

murong00 Apr 17, 2020

sijie Apr 16, 2020

murong00 Apr 17, 2020

murong00 commented Apr 17, 2020

murong00 commented Apr 17, 2020

murong00 commented Apr 20, 2020

[Issue 5935] Support multi pulsar clusters to use the same bk cluster #5985

[Issue 5935] Support multi pulsar clusters to use the same bk cluster #5985

Conversation

murong00 commented Jan 3, 2020 • edited

Motivation

Modifications

tuteng commented Jan 8, 2020

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

murong00 commented Jan 19, 2020

murong00 commented Feb 17, 2020

murong00 commented Feb 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sijie commented Feb 24, 2020

murong00 commented Feb 25, 2020 • edited

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

jiazhai commented Apr 2, 2020

murong00 commented Apr 3, 2020

murong00 commented Apr 9, 2020

murong00 commented Apr 14, 2020

jiazhai left a comment

Choose a reason for hiding this comment

murong00 commented Apr 14, 2020

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

murong00 commented Apr 17, 2020

murong00 commented Apr 17, 2020

murong00 commented Apr 20, 2020

murong00 commented Jan 3, 2020 •

edited

murong00 commented Feb 25, 2020 •

edited