You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/09/19 17:08:52 UTC
[GitHub] [pulsar] aloyszhang opened a new pull request, #17722: support setting bundle number for default namespace when set up cluster
aloyszhang opened a new pull request, #17722:
URL: https://github.com/apache/pulsar/pull/17722
### Motivation
In the actual production environment, users may only use one default namespace (public/default).
The number of bundles in this namespace is a default value of 16.
1. if the cluster has more than 16 brokers, some brokers will serve no traffic
2. if the topics in these bundles are not equal, the load will be unbalanced
In addition, the manually split bundle is complicated and auto-split may lead to unstable services.
We expect to set a relatively large number of bundles for the default namespace when the cluster is built.
### Modifications
support setting bundle number for default namespace when set up cluster metadata
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
ClusterMetadataSetupTest.testSetupBundleForNamespaces
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-not-needed`
(Please explain why)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r980926667
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -63,10 +65,17 @@
*/
public class PulsarClusterMetadataSetup {
+ private static final int DEFAULT_BUNDLE_NUMBER = 16;
+
private static class Arguments {
@Parameter(names = { "-c", "--cluster" }, description = "Cluster name", required = true)
private String cluster;
+ @Parameter(names = {"-bc", "--broker-conf"},
+ description = "Configuration file for Broker to get the default bundle numbers "
Review Comment:
> This could be confusing because if the namespace is already existing, it won't have any effect. Also it's not clear in the command line that this will be related to the public/default namespace.
Another approach could be to remove the 16 bundles in the hardcoded from and instead just use the default number of bundles which is set in broker.conf
As @merlimat said, the default namespace may already exist(because the `initialize-cluster-metadata` command can be executed repeatedly), at this point, parameter passed by `--default-namespace-bundles` will not take effect
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#issuecomment-1260884860
After discussing with @codelipenghui, getting bundle numbers for default namespaces from `broker.conf` has a disadvantage in the k8s environment(build zk container, then start a container to init cluster metadata and at this point, we can't access the broker.conf).
So, adding a new option like `--default-namespace-bundle-numbers` is better. @merlimat
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#issuecomment-1261995182
cc @codelipenghui
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r976004193
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -63,10 +66,18 @@
*/
public class PulsarClusterMetadataSetup {
+ private static final int DEFAULT_BUNDLE_NUMBER = 16;
+
private static class Arguments {
@Parameter(names = { "-c", "--cluster" }, description = "Cluster name", required = true)
private String cluster;
+ @Parameter(names = {"-bc", "--broker-conf"},
+ description = "Configuration file for Broker to get the default bundle numbers "
+ + "for public/default namespace")
+ private String brokerConfigFile =
+ Paths.get("").toAbsolutePath().normalize().toString() + "/conf/broker.conf";
Review Comment:
Do we need this value? This will break the `DEFAULT_BUNDLE_NUMBER`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r976593420
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -233,8 +244,21 @@ public static void main(String[] args) throws Exception {
System.exit(1);
}
+ int bundleNumberForDefaultNamespace = DEFAULT_BUNDLE_NUMBER;
try {
- initializeCluster(arguments);
+ ServiceConfiguration conf =
+ PulsarConfigurationLoader.create(arguments.brokerConfigFile, ServiceConfiguration.class);
+ if (conf != null && conf.getDefaultNumberOfNamespaceBundles() > 0) {
+ bundleNumberForDefaultNamespace = conf.getDefaultNumberOfNamespaceBundles();
+ }
+ } catch (Exception e) {
+ log.warn("Failed to load broker configuration file {}, use default number of bundles {}",
Review Comment:
`log.warn` may not print to console. It's better use `System.out.println` I think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r977197408
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -233,8 +244,21 @@ public static void main(String[] args) throws Exception {
System.exit(1);
}
+ int bundleNumberForDefaultNamespace = DEFAULT_BUNDLE_NUMBER;
try {
- initializeCluster(arguments);
+ ServiceConfiguration conf =
+ PulsarConfigurationLoader.create(arguments.brokerConfigFile, ServiceConfiguration.class);
+ if (conf != null && conf.getDefaultNumberOfNamespaceBundles() > 0) {
+ bundleNumberForDefaultNamespace = conf.getDefaultNumberOfNamespaceBundles();
+ }
+ } catch (Exception e) {
+ log.warn("Failed to load broker configuration file {}, use default number of bundles {}",
Review Comment:
> `log.warn` may not print to console. It's better use `System.out.println` I think
>
> Also I think that
>
> ```
> System.err.println("xxx");
> System.exit(1);
> ```
>
> is better. Without exit, user will create an namespace with default number of bundles, which is not the user excepted result. Then the user have to update the bundles manually. And if we exit, the user will have a chance to check the config file and try again. WDYT :)
This makes sense to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r976188993
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -63,10 +66,18 @@
*/
public class PulsarClusterMetadataSetup {
+ private static final int DEFAULT_BUNDLE_NUMBER = 16;
+
private static class Arguments {
@Parameter(names = { "-c", "--cluster" }, description = "Cluster name", required = true)
private String cluster;
+ @Parameter(names = {"-bc", "--broker-conf"},
+ description = "Configuration file for Broker to get the default bundle numbers "
+ + "for public/default namespace")
+ private String brokerConfigFile =
+ Paths.get("").toAbsolutePath().normalize().toString() + "/conf/broker.conf";
Review Comment:
This is an optional configuration. Users can set this if they want to set the bundle number for the `public/default` namespace at the point of initializing the cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r976593420
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -233,8 +244,21 @@ public static void main(String[] args) throws Exception {
System.exit(1);
}
+ int bundleNumberForDefaultNamespace = DEFAULT_BUNDLE_NUMBER;
try {
- initializeCluster(arguments);
+ ServiceConfiguration conf =
+ PulsarConfigurationLoader.create(arguments.brokerConfigFile, ServiceConfiguration.class);
+ if (conf != null && conf.getDefaultNumberOfNamespaceBundles() > 0) {
+ bundleNumberForDefaultNamespace = conf.getDefaultNumberOfNamespaceBundles();
+ }
+ } catch (Exception e) {
+ log.warn("Failed to load broker configuration file {}, use default number of bundles {}",
Review Comment:
`log.warn` may not print to console. It's better use `System.out.println` I think
Also I think that
```
System.err.println("xxx");
System.exit(1);
```
is better. Without exit, user will create an namespace with default number of bundles, which is not the user excepted result. Then the user have to update the bundles manually. And if we exit, the user will have a chance to check the config file and try again. WDYT :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#issuecomment-1260477257
@codelipenghui PTAL, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r980853248
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -63,10 +65,17 @@
*/
public class PulsarClusterMetadataSetup {
+ private static final int DEFAULT_BUNDLE_NUMBER = 16;
+
private static class Arguments {
@Parameter(names = { "-c", "--cluster" }, description = "Cluster name", required = true)
private String cluster;
+ @Parameter(names = {"-bc", "--broker-conf"},
+ description = "Configuration file for Broker to get the default bundle numbers "
Review Comment:
> In the previous implementation, the bundle number for the default namespaces `public/default` is hardcoded as 16.
> What we are interested in is providing an option to set the bundle number from the user side.
Make he bundle number for the default namespaces configurable make perfect sense to me.
> As you said, we can get other parameters from the `broker.conf`, but considering the compatibility(for now, all parameters are from the command line, by default, it has higher priority than the config file). I think we can keep the way it works now. WDYT?
I just wonder what's wrong with parameter like "--default-namespace-bundles"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r974821681
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/zookeeper/ClusterMetadataSetupTest.java:
##########
@@ -86,6 +88,32 @@ public void testReSetupClusterMetadata() throws Exception {
assertEquals(data1, data3);
}
+ public void testSetBundleNumberForDefaultNamespace() throws Exception {
+
+ String[] args = {
+ "--cluster", "testReSetupClusterMetadata-cluster",
+ "--metadata-store", "zk:127.0.0.1:" + localZkS.getZookeeperPort() + "/metastore",
+ "--configuration-store", "127.0.0.1:" + localZkS.getZookeeperPort() + "/configurationStore",
+ "--web-service-url", "http://127.0.0.1:8080",
+ "--web-service-url-tls", "https://127.0.0.1:8443",
+ "--broker-service-url", "pulsar://127.0.0.1:6650",
+ "--broker-service-url-tls","pulsar+ssl://127.0.0.1:6651",
+ "--bundle-numbers", "64",
Review Comment:
> Another approach could be to remove the 16 bundles in the hardcoded from and instead just use the default number of bundles which is set in `broker.conf`
This is a better way.
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/zookeeper/ClusterMetadataSetupTest.java:
##########
@@ -86,6 +88,32 @@ public void testReSetupClusterMetadata() throws Exception {
assertEquals(data1, data3);
}
+ public void testSetBundleNumberForDefaultNamespace() throws Exception {
+
+ String[] args = {
+ "--cluster", "testReSetupClusterMetadata-cluster",
+ "--metadata-store", "zk:127.0.0.1:" + localZkS.getZookeeperPort() + "/metastore",
+ "--configuration-store", "127.0.0.1:" + localZkS.getZookeeperPort() + "/configurationStore",
+ "--web-service-url", "http://127.0.0.1:8080",
+ "--web-service-url-tls", "https://127.0.0.1:8443",
+ "--broker-service-url", "pulsar://127.0.0.1:6650",
+ "--broker-service-url-tls","pulsar+ssl://127.0.0.1:6651",
+ "--bundle-numbers", "64",
Review Comment:
> Another approach could be to remove the 16 bundles in the hardcoded from and instead just use the default number of bundles which is set in `broker.conf`
This is a better way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r979672440
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -63,10 +65,17 @@
*/
public class PulsarClusterMetadataSetup {
+ private static final int DEFAULT_BUNDLE_NUMBER = 16;
+
private static class Arguments {
@Parameter(names = { "-c", "--cluster" }, description = "Cluster name", required = true)
private String cluster;
+ @Parameter(names = {"-bc", "--broker-conf"},
+ description = "Configuration file for Broker to get the default bundle numbers "
Review Comment:
Seems a bit confusing if we only use "DefaultNumberOfNamespaceBundles" from this config file. Most of other parameters in this `Arguments` appears in the config file too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#issuecomment-1251778827
checks in fork repo : https://github.com/aloyszhang/pulsar/pull/2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r976188993
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -63,10 +66,18 @@
*/
public class PulsarClusterMetadataSetup {
+ private static final int DEFAULT_BUNDLE_NUMBER = 16;
+
private static class Arguments {
@Parameter(names = { "-c", "--cluster" }, description = "Cluster name", required = true)
private String cluster;
+ @Parameter(names = {"-bc", "--broker-conf"},
+ description = "Configuration file for Broker to get the default bundle numbers "
+ + "for public/default namespace")
+ private String brokerConfigFile =
+ Paths.get("").toAbsolutePath().normalize().toString() + "/conf/broker.conf";
Review Comment:
This is an optional configuration. Users can set this if they want to set the bundle number for the `public/default` namespace.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r977196654
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -63,10 +66,18 @@
*/
public class PulsarClusterMetadataSetup {
+ private static final int DEFAULT_BUNDLE_NUMBER = 16;
+
private static class Arguments {
@Parameter(names = { "-c", "--cluster" }, description = "Cluster name", required = true)
private String cluster;
+ @Parameter(names = {"-bc", "--broker-conf"},
+ description = "Configuration file for Broker to get the default bundle numbers "
+ + "for public/default namespace")
+ private String brokerConfigFile =
+ Paths.get("").toAbsolutePath().normalize().toString() + "/conf/broker.conf";
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r979999215
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -63,10 +65,17 @@
*/
public class PulsarClusterMetadataSetup {
+ private static final int DEFAULT_BUNDLE_NUMBER = 16;
+
private static class Arguments {
@Parameter(names = { "-c", "--cluster" }, description = "Cluster name", required = true)
private String cluster;
+ @Parameter(names = {"-bc", "--broker-conf"},
+ description = "Configuration file for Broker to get the default bundle numbers "
Review Comment:
In the previous implementation, the bundle number for the default namespaces `public/default` is hardcoded as 16.
What we are interested in is providing an option to set the bundle number from the user side. Here we get from the `broker.conf`. So, I think it has nothing to do with other configurations.
BTW, we can get other parameters from the `broker.conf`, but considering the compatibility(for now, all parameters are from the command line, by default, it has higher priority than the config file).
So, I think we can keep the way it works now. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#issuecomment-1257631056
cc @merlimat
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r979999215
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -63,10 +65,17 @@
*/
public class PulsarClusterMetadataSetup {
+ private static final int DEFAULT_BUNDLE_NUMBER = 16;
+
private static class Arguments {
@Parameter(names = { "-c", "--cluster" }, description = "Cluster name", required = true)
private String cluster;
+ @Parameter(names = {"-bc", "--broker-conf"},
+ description = "Configuration file for Broker to get the default bundle numbers "
Review Comment:
In the previous implementation, the bundle number for the default namespaces `public/default` is hardcoded as 16.
What we are interested in is providing an option to set the bundle number from the user side. Here we get it from the `broker.conf`. So, I think it has nothing to do with other configurations.
> Most of other parameters in this Arguments appears in the config file too.
As you said, we can get other parameters from the `broker.conf`, but considering the compatibility(for now, all parameters are from the command line, by default, it has higher priority than the config file). I think we can keep the way it works now. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r979999215
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -63,10 +65,17 @@
*/
public class PulsarClusterMetadataSetup {
+ private static final int DEFAULT_BUNDLE_NUMBER = 16;
+
private static class Arguments {
@Parameter(names = { "-c", "--cluster" }, description = "Cluster name", required = true)
private String cluster;
+ @Parameter(names = {"-bc", "--broker-conf"},
+ description = "Configuration file for Broker to get the default bundle numbers "
Review Comment:
In the previous implementation, the bundle number for the default namespaces `public/default` is hardcoded as 16.
What we are interested in is providing an option to set the bundle number from the user side. Here we get from the `broker.conf`. So, I think it has nothing to do with other configurations.
BTW, we can get other parameters from the `broker.conf`, but considering the compatibility(for now, all parameters are from the command line, by default, it has higher priority than the config file). So, I think we can keep the way it works now. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
merlimat commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r974764210
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/zookeeper/ClusterMetadataSetupTest.java:
##########
@@ -86,6 +88,32 @@ public void testReSetupClusterMetadata() throws Exception {
assertEquals(data1, data3);
}
+ public void testSetBundleNumberForDefaultNamespace() throws Exception {
+
+ String[] args = {
+ "--cluster", "testReSetupClusterMetadata-cluster",
+ "--metadata-store", "zk:127.0.0.1:" + localZkS.getZookeeperPort() + "/metastore",
+ "--configuration-store", "127.0.0.1:" + localZkS.getZookeeperPort() + "/configurationStore",
+ "--web-service-url", "http://127.0.0.1:8080",
+ "--web-service-url-tls", "https://127.0.0.1:8443",
+ "--broker-service-url", "pulsar://127.0.0.1:6650",
+ "--broker-service-url-tls","pulsar+ssl://127.0.0.1:6651",
+ "--bundle-numbers", "64",
Review Comment:
This could be confusing because if the namespace is already existing, it won't have any effect. Also it's not clear in the command line that this will be related to the `public/default` namespace.
Another approach could be to remove the 16 bundles in the hardcoded from and instead just use the default number of bundles which is set in `broker.conf`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#issuecomment-1252375466
@merlimat PTAL, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#issuecomment-1251889213
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r976639985
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -63,10 +66,18 @@
*/
public class PulsarClusterMetadataSetup {
+ private static final int DEFAULT_BUNDLE_NUMBER = 16;
+
private static class Arguments {
@Parameter(names = { "-c", "--cluster" }, description = "Cluster name", required = true)
private String cluster;
+ @Parameter(names = {"-bc", "--broker-conf"},
+ description = "Configuration file for Broker to get the default bundle numbers "
+ + "for public/default namespace")
+ private String brokerConfigFile =
+ Paths.get("").toAbsolutePath().normalize().toString() + "/conf/broker.conf";
Review Comment:
Oh, I see, but I suggest using the `private String brokerConfigFile;`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang merged pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang merged PR #17722:
URL: https://github.com/apache/pulsar/pull/17722
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a diff in pull request #17722: support setting bundle number for default namespace when set up cluster
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on code in PR #17722:
URL: https://github.com/apache/pulsar/pull/17722#discussion_r980926667
##########
pulsar-broker/src/main/java/org/apache/pulsar/PulsarClusterMetadataSetup.java:
##########
@@ -63,10 +65,17 @@
*/
public class PulsarClusterMetadataSetup {
+ private static final int DEFAULT_BUNDLE_NUMBER = 16;
+
private static class Arguments {
@Parameter(names = { "-c", "--cluster" }, description = "Cluster name", required = true)
private String cluster;
+ @Parameter(names = {"-bc", "--broker-conf"},
+ description = "Configuration file for Broker to get the default bundle numbers "
Review Comment:
> This could be confusing because if the namespace is already existing, it won't have any effect. Also it's not clear in the command line that this will be related to the public/default namespace.
> Another approach could be to remove the 16 bundles in the hardcoded from and instead just use the default number of bundles which is set in broker.conf
As @merlimat said, the default namespace may already exist(because the `initialize-cluster-metadata` command can be executed repeatedly), at this point, parameter passed by `--default-namespace-bundles` will not take effect
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org