[Issue 8130] Refine BookKeeper metadata service uri when init metadata by murong00 · Pull Request #8269 · apache/pulsar

murong00 · 2020-10-15T11:00:17Z

Motivation

Fixes #8130

Modifications

rename --bookkeeper-metadata-service-uri by --existing-bk-metadata-service-uri to avoid misunderstanding
add some specification of this parameter in corresponding md files

murong00 · 2020-10-15T11:01:26Z

@BewareMyPower @jennifer88huang Please help to take a look about this change, thanks.

BewareMyPower · 2020-10-15T13:26:46Z

LGTM. Just one point, I think it's better to change

--existing-bk-metadata-service-uri "zk+null://bk1:2181;bk2:2181/ledgers"

to zk1:2181,zk2:2181 to emphasize that the metadata store is ZK (not Etcd,etc.) or just host1:2181,host2:2181 to be consistent with other params.

Jennifer88huang-zz · 2020-10-15T14:59:13Z

 > --broker-service-url-tls pulsar+ssl://host1:6651,host2:6651,host3:6651
 > ```

+> If you want to use an existing BookKeeper cluster, you can add `--existing-bk-metadata-service-uri` flag at the mean time like below:


Suggested change

> If you want to use an existing BookKeeper cluster, you can add `--existing-bk-metadata-service-uri` flag at the mean time like below:

> If you want to use an existing BookKeeper cluster, you can add the `--existing-bk-metadata-service-uri` flag as follows:

Jennifer88huang-zz · 2020-10-15T15:03:55Z

+> --broker-service-url pulsar://host1:6650,host2:6650,host3:6650 \
+> --broker-service-url-tls pulsar+ssl://host1:6651,host2:6651,host3:6651
+> ```
+> The metadata service uri of existing BookKeeper cluster can be fetched via `bin/bookkeeper shell whatisinstanceid` command. Note that the value need to be enclosed in double quotes since the multi metadata service uri is semicolon separated.


Suggested change

> The metadata service uri of existing BookKeeper cluster can be fetched via `bin/bookkeeper shell whatisinstanceid` command. Note that the value need to be enclosed in double quotes since the multi metadata service uri is semicolon separated.

> You can obtain the metadata service URI of the existing BookKeeper cluster by using the `bin/bookkeeper shell whatisinstanceid` command. You must enclose the value in double quotes since the multiple metadata service URIs are separated with semicolons.

thanks, done.

Jennifer88huang-zz · 2020-10-15T15:06:34Z

 |`-c` , `--cluster`|Cluster name||
-|`--configuration-store`|The configuration store quorum connection string||
+|`-cs` , `--configuration-store`|The configuration store quorum connection string||
+|`--existing-bk-metadata-service-uri`|The metadata service uri of existing BookKeeper cluster you want to use||


Suggested change

|`--existing-bk-metadata-service-uri`|The metadata service uri of existing BookKeeper cluster you want to use||

|`--existing-bk-metadata-service-uri`|The metadata service URI of the existing BookKeeper cluster you want to use||

Jennifer88huang-zz · 2020-10-15T15:07:31Z

+|`-cs` , `--configuration-store`|The configuration store quorum connection string||
+|`--existing-bk-metadata-service-uri`|The metadata service uri of existing BookKeeper cluster you want to use||
+|`-h` , `--help`|Cluster name|false|
+|`--initial-num-stream-storage-containers`|Num storage containers of BookKeeper stream storage|16|


what does "Num" mean?

"Num" is short for number.

Jennifer88huang-zz · 2020-10-15T15:07:47Z

 |`-uw` , `--web-service-url`|The web service URL for the new cluster||
 |`-tw` , `--web-service-url-tls`|The web service URL for the new cluster with TLS encryption||
 |`-zk` , `--zookeeper`|The local ZooKeeper quorum connection string||
+|`--zookeeper-session-timeout-ms`|Local zookeeper session timeout ms|30000|


What does "ms" stand for in "Local zookeeper session timeout ms"?

ms is the time unit which is short for millisecond.

Thanks for your clarification. Maybe we can add it in the description, so it will be clear to users.

Jennifer88huang-zz · 2020-10-15T15:13:15Z

@murong00 I've left some minor comments. Any issue, feel free to ping me.

murong00 · 2020-10-16T01:15:35Z

LGTM. Just one point, I think it's better to change

--existing-bk-metadata-service-uri "zk+null://bk1:2181;bk2:2181/ledgers"

to zk1:2181,zk2:2181 to emphasize that the metadata store is ZK (not Etcd,etc.) or just host1:2181,host2:2181 to be consistent with other params.

@BewareMyPower I agree with you, however the result of command bin/bookkeeper shell whatisinstanceid is semicolon separated in multi-hosts case (due to bookkeeper only support semicolon separated values currently, you can refer to #6998 for some detail), if we replace semicolon with comma the BkClient will fail to be created, so I just keep its result here.

Jennifer88huang-zz · 2020-10-16T02:36:29Z

 |`-c` , `--cluster`|Cluster name||
 |`-cs` , `--configuration-store`|The configuration store quorum connection string||
-|`--existing-bk-metadata-service-uri`|The metadata service uri of existing BookKeeper cluster you want to use||
+|`--existing-bk-metadata-service-uri`|The metadata service URI of the existing BookKeeper cluster you want to us||


Suggested change

|`--existing-bk-metadata-service-uri`|The metadata service URI of the existing BookKeeper cluster you want to us||

|`--existing-bk-metadata-service-uri`|The metadata service URI of the existing BookKeeper cluster that you want to use||

Jennifer88huang-zz · 2020-10-16T02:37:28Z

-|`--existing-bk-metadata-service-uri`|The metadata service uri of existing BookKeeper cluster you want to use||
+|`--existing-bk-metadata-service-uri`|The metadata service URI of the existing BookKeeper cluster you want to us||
 |`-h` , `--help`|Cluster name|false|
 |`--initial-num-stream-storage-containers`|Num storage containers of BookKeeper stream storage|16|


So shall we use "The number of storage containers of BookKeeper stream storage"

Jennifer88huang-zz · 2020-10-16T02:38:01Z

+|`--existing-bk-metadata-service-uri`|The metadata service URI of the existing BookKeeper cluster you want to us||
 |`-h` , `--help`|Cluster name|false|
 |`--initial-num-stream-storage-containers`|Num storage containers of BookKeeper stream storage|16|
 |`--initial-num-transaction-coordinators`|Num transaction coordinators will assigned in cluster|16|


The number of transaction coordinators assigned in a cluster

Jennifer88huang-zz · 2020-10-16T02:42:57Z

 |`-uw` , `--web-service-url`|The web service URL for the new cluster||
 |`-tw` , `--web-service-url-tls`|The web service URL for the new cluster with TLS encryption||
 |`-zk` , `--zookeeper`|The local ZooKeeper quorum connection string||
+|`--zookeeper-session-timeout-ms`|Local zookeeper session timeout ms|30000|


murong00 · 2020-10-16T04:03:45Z

@jennifer88huang I hava addressed your comments, PTAL again.

Jennifer88huang-zz

well done, thank you

murong00 · 2020-10-16T08:37:54Z

/pulsarbot run-failure-checks

Huanli-Meng

LGTM for doc-related updates.

apache#8269) Motivation Fixes apache#8130 Modifications rename --bookkeeper-metadata-service-uri by --existing-bk-metadata-service-uri to avoid misunderstanding add some specification of this parameter in corresponding md files * Refine BookKeeper metadata service uri when init metadata * address the review comments * address Jennifer's comment

--- *Motivation* #8269 change the arguments name which causes the setup metadata command is not compatible with old versions. For keeping the compatibility of the setup metadata command, we should avoid deleting the existing arguments. If we need to change it, it's better to keep the old arguments and mark it deprecates and hide them.

Refine BookKeeper metadata service uri when init metadata

1232d2d

murong00 requested a review from sijie October 15, 2020 11:01

Jennifer88huang-zz reviewed Oct 15, 2020

View reviewed changes

Jennifer88huang-zz assigned murong00 Oct 15, 2020

Jennifer88huang-zz added this to the 2.7.0 milestone Oct 15, 2020

Jennifer88huang-zz added area/cli component/deploy labels Oct 15, 2020

address the review comments

04bb1b3

Jennifer88huang-zz reviewed Oct 16, 2020

View reviewed changes

address Jennifer's comment

d4477c3

Jennifer88huang-zz approved these changes Oct 16, 2020

View reviewed changes

Huanli-Meng approved these changes Oct 16, 2020

View reviewed changes

jiazhai approved these changes Oct 21, 2020

View reviewed changes

jiazhai merged commit 3147e9e into apache:master Oct 21, 2020

zymap mentioned this pull request Dec 15, 2020

Fix the metadata setup compatibility issue #8959

Merged

	> If you want to use an existing BookKeeper cluster, you can add `--existing-bk-metadata-service-uri` flag at the mean time like below:
	> If you want to use an existing BookKeeper cluster, you can add the `--existing-bk-metadata-service-uri` flag as follows:

	> The metadata service uri of existing BookKeeper cluster can be fetched via `bin/bookkeeper shell whatisinstanceid` command. Note that the value need to be enclosed in double quotes since the multi metadata service uri is semicolon separated.
	> You can obtain the metadata service URI of the existing BookKeeper cluster by using the `bin/bookkeeper shell whatisinstanceid` command. You must enclose the value in double quotes since the multiple metadata service URIs are separated with semicolons.

	\|`--existing-bk-metadata-service-uri`\|The metadata service uri of existing BookKeeper cluster you want to use\|\|
	\|`--existing-bk-metadata-service-uri`\|The metadata service URI of the existing BookKeeper cluster you want to use\|\|

	\|`--zookeeper-session-timeout-ms`\|Local zookeeper session timeout ms\|30000\|
	\|`--zookeeper-session-timeout-ms`\|The local ZooKeeper session timeout. The time unit is in millisecond(ms).\|30000\|

Uh oh!

Conversation

murong00 commented Oct 15, 2020

Motivation

Modifications

Uh oh!

murong00 commented Oct 15, 2020

Uh oh!

BewareMyPower commented Oct 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jennifer88huang-zz Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jennifer88huang-zz commented Oct 15, 2020

Uh oh!

murong00 commented Oct 16, 2020

Uh oh!

Jennifer88huang-zz Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murong00 commented Oct 16, 2020

Uh oh!

Jennifer88huang-zz left a comment

Choose a reason for hiding this comment

Uh oh!

murong00 commented Oct 16, 2020

Uh oh!

Huanli-Meng left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Jennifer88huang-zz Oct 15, 2020 •

edited

Loading

Jennifer88huang-zz Oct 16, 2020 •

edited

Loading