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.

Sign up for GitHub

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

Jump to bottom

[fix][broker] The configuration loadBalancerNamespaceMaximumBundles is invalid #16552

Merged
merged 7 commits into from
Jul 18, 2022
Merged

[fix][broker] The configuration loadBalancerNamespaceMaximumBundles is invalid #16552

merged 7 commits into from
Jul 18, 2022

Conversation

lordcheng10
Copy link
Contributor

@lordcheng10 lordcheng10 commented Jul 12, 2022 •
edited
Loading

Motivation

The configuration loadBalancerNamespaceMaximumBundles is invalid.

example to illustrate:

  1. Configure loadBalancerNamespaceMaximumBundles=128
  2. The current number of bundles in the namespace is 120
  3. According to the current logic, after executing the method findBundlesToSplit, 40 bundles may be found that need to be split;
  4. After these bundles, the number of bundles in the namespace will become 120+40=160 > loadBalancerNamespaceMaximumBundles=128

modified logic：

  1. When judging whether the number of bundles exceeds the configuration, it is necessary to add the already selected bundles：

image

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@Demogorgon314
Copy link
Member

Can you add a unit test to cover this case?

@codelipenghui codelipenghui added this to the 2.11.0 milestone Jul 13, 2022
@codelipenghui codelipenghui added type/bug The PR fixed a bug or issue reported a bug area/broker release/2.10.2 release/2.9.4 labels Jul 13, 2022
@codelipenghui
Copy link
Contributor

@lordcheng10 Could you please provide more details about the BUG? so that the reviewers can easily understand what the issue is that the PR wants to fix.

@lordcheng10
Copy link
Contributor Author

lordcheng10 commented Jul 13, 2022 •
edited
Loading

@lordcheng10 Could you please provide more details about the BUG? so that the reviewers can easily understand what the issue is that the PR wants to fix.

Sorry, I didn't describe clearly. @codelipenghui

I'll give an example to illustrate:

  1. Configure loadBalancerNamespaceMaximumBundles=128
  2. The current number of bundles in the namespace is 120
  3. According to the current logic, after executing the method findBundlesToSplit, 40 bundles may be found that need to be split;
  4. After these bundles, the number of bundles in the namespace will become 120+40=160 > loadBalancerNamespaceMaximumBundles=128

@lordcheng10
Copy link
Contributor Author

Can you add a unit test to cover this case?

OK

@Technoboy-
Copy link
Contributor

@lordcheng10 Could you please provide more details about the BUG? so that the reviewers can easily understand what the issue is that the PR wants to fix.

Sorry, I didn't describe clearly. @codelipenghui

I'll give an example to illustrate:

  1. Configure loadBalancerNamespaceMaximumBundles=128
  2. The current number of bundles in the namespace is 120
  3. According to the current logic, after executing the method findBundlesToSplit, 40 bundles may be found that need to be split;
  4. After these bundles, the number of bundles in the namespace will become 120+40=160 > loadBalancerNamespaceMaximumBundles=128

Make sense.

@lordcheng10
Copy link
Contributor Author

@lordcheng10 Could you please provide more details about the BUG? so that the reviewers can easily understand what the issue is that the PR wants to fix.

Sorry, I didn't describe clearly. @codelipenghui
I'll give an example to illustrate:

  1. Configure loadBalancerNamespaceMaximumBundles=128
  2. The current number of bundles in the namespace is 120
  3. According to the current logic, after executing the method findBundlesToSplit, 40 bundles may be found that need to be split;
  4. After these bundles, the number of bundles in the namespace will become 120+40=160 > loadBalancerNamespaceMaximumBundles=128

Make sense.

When judging whether the number of bundles exceeds the configuration, it is necessary to add the already selected bundles. @codelipenghui @Technoboy-

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 15, 2022
@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

2 similar comments
@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

lordcheng10 commented Jul 17, 2022 •
edited
Loading

Can you add a unit test to cover this case?

added a unit test : testLoadBalancerNamespaceMaximumBundles

PTAL,thanks! @Demogorgon314 @codelipenghui

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

2 similar comments
@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

9 similar comments
@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

/pulsarbot run-failure-checks

@lordcheng10
Copy link
Contributor Author

@codelipenghui @aloyszhang PTAL,thanks!

@lordcheng10 lordcheng10 mentioned this pull request Jul 18, 2022
Merged
4 tasks
@codelipenghui codelipenghui merged commit 5698b08 into apache:master Jul 18, 2022
codelipenghui pushed a commit that referenced this pull request Jul 25, 2022
@lordcheng10 @codelipenghui
[fix][broker] The configuration loadBalancerNamespaceMaximumBundles i…
a0133e9 
…s invalid (#16552)

(cherry picked from commit 5698b08)
@mattisonchao mattisonchao added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Jul 25, 2022
mattisonchao pushed a commit that referenced this pull request Jul 25, 2022
@lordcheng10 @mattisonchao
[fix][broker] The configuration loadBalancerNamespaceMaximumBundles i…
7a9bd8f 
…s invalid (#16552)

(cherry picked from commit 5698b08)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Aug 16, 2022
@lordcheng10 @nicoloboschi
[fix][broker] The configuration loadBalancerNamespaceMaximumBundles i…
2365a03 
…s invalid (apache#16552)

(cherry picked from commit 5698b08)
(cherry picked from commit a0133e9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Technoboy- Technoboy- Technoboy- approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@Demogorgon314 Demogorgon314 Demogorgon314 approved these changes

@aloyszhang aloyszhang aloyszhang approved these changes

Assignees

@lordcheng10 lordcheng10

Labels
area/broker cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.9.4 release/2.10.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
2.11.0
Development

Successfully merging this pull request may close these issues.
6 participants
@lordcheng10 @Demogorgon314 @codelipenghui @Technoboy- @aloyszhang @mattisonchao