Skip to content
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

support setting bundle number for default namespace when set up cluster #17722

Merged
merged 1 commit into from
Sep 30, 2022

Conversation

aloyszhang
Copy link
Contributor

@aloyszhang aloyszhang commented Sep 19, 2022

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 setting up cluster metadata
and the bundle number is the defaultNumberOfNamespaceBundles from broker.conf
and the bundle number passed by a new option like --default-namespacs-bundles-numbers

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

  • doc-not-needed
    (Please explain why)

PR in for repository

aloyszhang#2

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 19, 2022
"--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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@aloyszhang aloyszhang Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@aloyszhang
Copy link
Contributor Author

checks in fork repo : aloyszhang#2

@aloyszhang aloyszhang requested a review from merlimat September 20, 2022 06:11
@aloyszhang
Copy link
Contributor Author

/pulsarbot run-failure-checks

@aloyszhang aloyszhang self-assigned this Sep 20, 2022
@aloyszhang
Copy link
Contributor Author

@merlimat PTAL, thanks

@aloyszhang aloyszhang requested a review from nodece September 21, 2022 08:24
@aloyszhang aloyszhang force-pushed the bundles branch 2 times, most recently from 707378a to f58b11b Compare September 22, 2022 06:02
@aloyszhang
Copy link
Contributor Author

cc @merlimat

@aloyszhang
Copy link
Contributor Author

@codelipenghui PTAL, thanks

@aloyszhang
Copy link
Contributor Author

aloyszhang commented Sep 28, 2022

After discussing with @codelipenghui, getting bundle numbers for default namespaces from broker.conf has a disadvantage in the k8s environment(In k8s, we build pulsar as follows: build zk container, then start a container to init cluster metadata, at this point, we can't access the broker.conf at all).

So, adding a new option like --default-namespace-bundle-numbers is better. @merlimat

@aloyszhang aloyszhang requested a review from Jason918 September 29, 2022 08:23
@aloyszhang
Copy link
Contributor Author

cc @codelipenghui

@aloyszhang aloyszhang merged commit 5e8902c into apache:master Sep 30, 2022
@aloyszhang aloyszhang deleted the bundles branch November 30, 2022 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants