-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Pulsar-Perf] Allow configuring metadataStoreUrl
in pulsar-perf managed-ledger
#14145
Conversation
…naged-ledger` Motivation `zookeeperServers` need to be renamed to `metadataStoreUrl` Modifications * Add `metadataStoreUrl` parameter to ManagedLedgerWriter of the pulsar perf. Signed-off-by: Zike Yang <zkyang@streamnative.io>
pulsar-testclient/src/main/java/org/apache/pulsar/testclient/ManagedLedgerWriter.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Zike Yang <zkyang@streamnative.io>
Signed-off-by: Zike Yang <zkyang@streamnative.io>
…dger` Add doc for apache#14145 Signed-off-by: Zike Yang <zkyang@streamnative.io>
ClientConfiguration bkConf = new ClientConfiguration(); | ||
bkConf.setUseV2WireProtocol(true); | ||
bkConf.setAddEntryTimeout(30); | ||
bkConf.setReadEntryTimeout(30); | ||
bkConf.setThrottleValue(0); | ||
bkConf.setNumChannelsPerBookie(arguments.maxConnections); | ||
bkConf.setZkServers(arguments.zookeeperServers); | ||
bkConf.setMetadataServiceUri(arguments.metadataStoreUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that only the param arguments.metadataStoreUrl
is valid, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
@@ -151,6 +156,12 @@ public static void main(String[] args) throws Exception { | |||
PerfClientUtils.exit(-1); | |||
} | |||
|
|||
if (arguments.metadataStoreUrl == null && arguments.zookeeperServers == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the param arguments.zookeeperServers
still be useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to make it backward compatible.
pulsar-testclient/src/main/java/org/apache/pulsar/testclient/ManagedLedgerWriter.java
Show resolved
Hide resolved
Signed-off-by: Zike Yang <zkyang@streamnative.io>
public String zookeeperServers; | ||
|
||
@Parameter(names = {"-md", | ||
"--metadata-store"}, description = "Metadata store service url. eg: zk:my-zk:2181") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"--metadata-store"}, description = "Metadata store service url. eg: zk:my-zk:2181") | |
"--metadata-store"}, description = "Metadata store service URL. For example: zk:my-zk:2181") |
Please capitalize proper nouns correctly. Details: https://docs.google.com/document/d/1lc5j4RtuLIzlEYCBo97AC8-U_3Erzs_lxpkDuseU0n4/edit#bookmark=id.je5thtkj42cb
…naged-ledger` (apache#14145) * [Pulsar-Perf] Allow configuring `metadataStoreUrl` in `pulsar-perf managed-ledger` Motivation `zookeeperServers` need to be renamed to `metadataStoreUrl` Modifications * Add `metadataStoreUrl` parameter to ManagedLedgerWriter of the pulsar perf. Signed-off-by: Zike Yang <zkyang@streamnative.io> * Make zookeeperServers hidden. Signed-off-by: Zike Yang <zkyang@streamnative.io> * Add null checker. Signed-off-by: Zike Yang <zkyang@streamnative.io> * Add deprecated. Signed-off-by: Zike Yang <zkyang@streamnative.io>
Master Issue: #13760
Motivation
As per #13760,
zookeeperServers
need to be renamed tometadataStoreUrl
.Modifications
metadataStoreUrl
parameter to ManagedLedgerWriter of the pulsar perf.Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation
doc-required