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

[fix][broker] Fix default bundle size used while setting bookie affinity #20250

Merged

Conversation

syk-coder
Copy link
Contributor

@syk-coder syk-coder commented May 8, 2023

Fixes #20194

Motivation

Motivation & Modifications:

After creating the namespace, if the bookie affinity is set for the namespaces, before setting the namespace policies, then for setting bookie Affinity currently the code directly uses the default bundle of size 1 instead of reading the configuration to get the default bundle size while creating the namespaces.

The change fixes that and uses the user-provided value from the broker configurations to set the bundles.

Verifying this change

  • Make sure that the change passes the CI checks.

The tests are already in place to create the bookie affinity group in BrokerBookieIsolationTest.java . So Just added a check to get the bundle size that was missing earlier.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 8, 2023
@syk-coder syk-coder marked this pull request as ready for review May 8, 2023 09:07
@Technoboy- Technoboy- added this to the 3.1.0 milestone May 9, 2023
@Technoboy- Technoboy- closed this May 9, 2023
@Technoboy- Technoboy- reopened this May 9, 2023
Copy link
Contributor

@Technoboy- Technoboy- left a comment

Choose a reason for hiding this comment

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

Could you also help change orElse to orElseGet at line 1713 and 1763

@syk-coder
Copy link
Contributor Author

syk-coder commented May 9, 2023

Could you also help change orElse to orElseGet at line 1713 and 1763

Even at those places and the remaining places where we are creating new LocalPolicies() we are not using the bundle size from the configuration. Should I change it to have the bundle size supplied by the user from the configuration? Should we do it in this PR or shall we create another bug and link it and fix them there?

@syk-coder
Copy link
Contributor Author

syk-coder commented May 9, 2023

Could you also help change orElse to orElseGet at line 1713 and 1763

Even at those places and the remaining places where we are creating new LocalPolicies() we are not using the bundle size from the configuration. Should I change it to have the bundle size supplied by the user from the configuration? Should we do it in this PR or shall we create another bug and link it and fix them there?

As this PR is already approved, created a new issue(#20278 ) for this and will push the changes in a new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] While Setting BookieAffinity group, not using the broker config to set the default number of bundles
4 participants