Visitar URL original
[improve][broker] PIP-192 Excluded bundles with isolation policy or anti-affinity-group policy from topk load bundles by heesung-sohn · Pull Request #19742 · apache/pulsar · GitHub
Skip to content

[improve][broker] PIP-192 Excluded bundles with isolation policy or anti-affinity-group policy from topk load bundles - #19742

Merged
Demogorgon314 merged 2 commits into
apache:masterfrom
heesung-sohn:pip-192-topk-exclude-policies
Mar 21, 2023
Merged

[improve][broker] PIP-192 Excluded bundles with isolation policy or anti-affinity-group policy from topk load bundles#19742
Demogorgon314 merged 2 commits into
apache:masterfrom
heesung-sohn:pip-192-topk-exclude-policies

Conversation

@heesung-sohn

@heesung-sohn heesung-sohn commented Mar 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Master Issue: #16691

Motivation

Raising a PR to implement #16691.

Bundles with isolation policies or anti-affinity-group policies are not ideal targets to auto-unload as destination brokers are limited.

Modifications

This PR

  • Excluded bundles with isolation policy or anti-affinity-group policy from topk load bundles
  • Introduced a config loadBalancerSheddingBundlesWithPoliciesEnabled to control this behavior.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • *Updated unit tests.

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
  • Anything that affects deployment

Documentation

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

We will have separate PRs to update the Doc later.

Matching PR in forked repository

PR in forked repository: heesung-sohn#38

@github-actions

github-actions Bot commented Mar 7, 2023

Copy link
Copy Markdown

@heesung-sn Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@github-actions github-actions Bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Mar 7, 2023
@heesung-sohn
heesung-sohn force-pushed the pip-192-topk-exclude-policies branch from 2f091fc to 48d9723 Compare March 7, 2023 21:49
@heesung-sohn heesung-sohn mentioned this pull request Mar 7, 2023
Closed
gaoran10
gaoran10 reviewed Mar 9, 2023
View reviewed changes

@gaoran10 gaoran10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we enable this config loadBalancerSheddingBundlesWithPoliciesEnabled , maybe we need to exclude brokers by isolation policies while transferring bundles.

@heesung-sohn

heesung-sohn commented Mar 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

If we enable this config loadBalancerSheddingBundlesWithPoliciesEnabled , maybe we need to exclude brokers by isolation policies while transferring bundles.

This config targets bundles that configure isolation or anti-affinity policies. This PR filters out such bundles in the TransferShedder as well. (This PR needs to apply the IsolationPolicy support logic from this PR #19592, when merged.)

if (!isLoadBalancerSheddingBundlesWithPoliciesEnabled
                && allocationPolicies.areIsolationPoliciesPresent(namespace)) {
            return false;

@gaoran10

gaoran10 commented Mar 10, 2023

Copy link
Copy Markdown
Contributor

This config targets bundles that configure isolation or anti-affinity policies. This PR filters out such bundles in the TransferShedder as well. (This PR needs to apply the IsolationPolicy support logic from this PR #19592, when merged.)

Got it, thanks.

@heesung-sohn
heesung-sohn force-pushed the pip-192-topk-exclude-policies branch 3 times, most recently from 57f5bb7 to 17f6bcd Compare March 14, 2023 20:43
@heesung-sohn

heesung-sohn commented Mar 14, 2023

Copy link
Copy Markdown
Contributor Author

@Demogorgon314 @gaoran10 @Technoboy-
I rebased the code. Please continue the review.

gaoran10
gaoran10 reviewed Mar 16, 2023
View reviewed changes
Comment thread ...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/TransferShedder.java Outdated
pulsar.getNamespaceService().getNamespaceBundleFactory().getBundle(namespace, bundleRange);

if (!canTransferWithIsolationPoliciesToBroker(
if (!isLoadBalancerSheddingBundlesWithPoliciesEnabled

@gaoran10 gaoran10 Mar 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm a little confused. When getting TopKBundles if isLoadBalancerSheddingBundlesWithPoliciesEnabled is false and the bundle has an affinity(anti) policy, the bundle will be discarded, maybe we only need to check whether the bundle matches the policy or not.

I'm not sure if this table is correct.

IsLoadBalancerSheddingBundlesWithPoliciesEnabled HasPolicy MatchPolicy Auto-Unload
false true true false
false true false false
false false -- true
true true true true
true true false false
true false -- true

@heesung-sohn heesung-sohn Mar 18, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The above table is correct. Thank you for this catch. Updated the code.

@heesung-sohn
heesung-sohn force-pushed the pip-192-topk-exclude-policies branch from 17f6bcd to e6adf65 Compare March 18, 2023 01:10
Heesung Sohn added 2 commits March 18, 2023 20:40
@heesung-sohn
heesung-sohn force-pushed the pip-192-topk-exclude-policies branch from e6adf65 to 5a937ae Compare March 19, 2023 03:49
@Demogorgon314 Demogorgon314 added ready-to-test area/broker type/refactor Code or documentation refactors. e.g. refactor code structure or methods to improve code readability labels Mar 19, 2023
@Demogorgon314 Demogorgon314 reopened this Mar 19, 2023
@heesung-sohn

heesung-sohn commented Mar 21, 2023

Copy link
Copy Markdown
Contributor Author

@Demogorgon314 plz merge this pr if looking good.

@Demogorgon314
Demogorgon314 merged commit 30d2469 into apache:master Mar 21, 2023
@heesung-sohn
heesung-sohn deleted the pip-192-topk-exclude-policies branch April 2, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gaoran10 gaoran10 gaoran10 approved these changes
@Demogorgon314 Demogorgon314 Demogorgon314 approved these changes
@Technoboy- Technoboy- Awaiting requested review from Technoboy-

Assignees

@heesung-sohn heesung-sohn

Labels

area/broker doc-not-needed Your PR changes do not impact docs ready-to-test type/refactor Code or documentation refactors. e.g. refactor code structure or methods to improve code readability

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@heesung-sohn @gaoran10 @Demogorgon314