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

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

Merged
merged 2 commits into from
Mar 21, 2023
Merged

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

merged 2 commits into from
Mar 21, 2023

Conversation

heesung-sn
Copy link
Contributor

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

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-sn#38

@github-actions
Copy link

github-actions bot commented Mar 7, 2023

@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-sn heesung-sn force-pushed the pip-192-topk-exclude-policies branch from 2f091fc to 48d9723 Compare March 7, 2023 21:49
@heesung-sn heesung-sn mentioned this pull request Mar 7, 2023
Closed
gaoran10
gaoran10 reviewed Mar 9, 2023
View reviewed changes
Copy link
Contributor

@gaoran10 gaoran10 left a comment

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-sn
Copy link
Contributor Author

heesung-sn commented Mar 9, 2023 •
edited
Loading

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
Copy link
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-sn heesung-sn force-pushed the pip-192-topk-exclude-policies branch 3 times, most recently from 57f5bb7 to 17f6bcd Compare March 14, 2023 20:43
@heesung-sn
Copy link
Contributor Author

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

gaoran10
gaoran10 reviewed Mar 16, 2023
View reviewed changes
...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/TransferShedder.java Outdated
String namespace = LoadManagerShared.getNamespaceNameFromBundleName(bundle);
final String bundleRange = LoadManagerShared.getBundleRangeFromBundleName(bundle);
NamespaceBundle namespaceBundle =
pulsar.getNamespaceService().getNamespaceBundleFactory().getBundle(namespace, bundleRange);

if (!canTransferWithIsolationPoliciesToBroker(
if (!isLoadBalancerSheddingBundlesWithPoliciesEnabled
Copy link
Contributor

@gaoran10 gaoran10 Mar 16, 2023 •
edited
Loading

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

Copy link
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-sn heesung-sn force-pushed the pip-192-topk-exclude-policies branch from 17f6bcd to e6adf65 Compare March 18, 2023 01:10
@heesung-sn heesung-sn 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-sn
Copy link
Contributor Author

@Demogorgon314 plz merge this pr if looking good.

@Demogorgon314 Demogorgon314 merged commit 30d2469 into apache:master Mar 21, 2023
@heesung-sn heesung-sn 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

@Demogorgon314 Demogorgon314 Demogorgon314 approved these changes

@gaoran10 gaoran10 gaoran10 approved these changes

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

Assignees

@heesung-sn heesung-sn

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-sn @gaoran10 @Demogorgon314