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

[improve][admin] PIP-369 Introduce unload flag in ns-isolation-policy set call #23120

Merged
merged 10 commits into from
Sep 1, 2024

Conversation

grssam
Copy link
Contributor

@grssam grssam commented Aug 2, 2024

Implementation PR for the PIP 369 "Flag based selective unload on changing ns-isolation-policy" #23116

Modifications

Added an unload-scope flag in the ns-isolation-policy set call to control the behavior of which namespaces to unload as part of the set call in a more granular manner.

As this PR is targetted for 3.0 LTS as well, keeping the changes backward compatible. i.e. the flag unload-scope is optional. Default value is all_matching which means all namespaces matching the namespaces regex being set in the policy set call.

There will be a followup PR which will be adding the following breaking changes:

  • Making the default value of the flag to be changed i.e. only the newly added or now-removed namespace regex would be used to unload the namespaces
  • The meaning of all_matching will change to cover all namespaces matching both the previous and the new namespace regex values. The idea is that if immediate placement correction is needed, we should also unload the namespaces which do not belong to the isolation group anymore.

Verifying this change

This change added tests and can be verified as follows:

  • Added and made changes to integration tests covering this feature. All three unload scopes are tried and validation is done that only the relevant namespaces are unloaded

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

PR in forked repo - grssam#1

Copy link

github-actions bot commented Aug 2, 2024

@grssam 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-required Your PR changes impact docs and you will update later. and removed doc-label-missing labels Aug 2, 2024
@grssam grssam changed the title [WIP][feat][admin] Introduce unload flag in ns-isolation-policy set call [feat][admin] PIP-369 Introduce unload flag in ns-isolation-policy set call Aug 19, 2024
@grssam grssam changed the title [feat][admin] PIP-369 Introduce unload flag in ns-isolation-policy set call [improve][admin] PIP-369 Introduce unload flag in ns-isolation-policy set call Aug 19, 2024
@grssam grssam marked this pull request as ready for review August 19, 2024 06:06
@grssam
Copy link
Contributor Author

grssam commented Aug 22, 2024

Please add labels for 3.0.7 release as I would like this to be present in the LTS version.

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM. Outstanding work @grssam! Thanks for contributing.

@grssam
Copy link
Contributor Author

grssam commented Aug 28, 2024

Not able to rerun the failed CI pipeline, but all tests passed in my repo - grssam#1 and the new tests that I added are part of "Broker Group 3" only.

@lhotari
Copy link
Member

lhotari commented Aug 28, 2024

Not able to rerun the failed CI pipeline, but all tests passed in my repo - grssam#1 and the new tests that I added are part of "Broker Group 3" only.

@grssam Pulsar CI, you can trigger a rerun by adding a comment "/pulsarbot rerun-failure-checks" to this PR. It requires that a possible previous workflow run has completed.

@grssam
Copy link
Contributor Author

grssam commented Aug 28, 2024

/pulsarbot rerun-failure-checks

@grssam
Copy link
Contributor Author

grssam commented Aug 28, 2024

/pulsarbot rerun-failure-checks

@nodece nodece self-requested a review August 29, 2024 08:51
@grssam
Copy link
Contributor Author

grssam commented Aug 29, 2024

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2024

Codecov Report

Attention: Patch coverage is 83.78378% with 6 lines in your changes missing coverage. Please review.

Project coverage is 74.55%. Comparing base (bbc6224) to head (746f072).
Report is 603 commits behind head on master.

Files with missing lines Patch % Lines
...cies/data/NamespaceIsolationPolicyUnloadScope.java 50.00% 4 Missing ⚠️
.../pulsar/admin/cli/CmdNamespaceIsolationPolicy.java 0.00% 1 Missing ⚠️
...on/policies/impl/NamespaceIsolationPolicyImpl.java 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23120      +/-   ##
============================================
+ Coverage     73.57%   74.55%   +0.98%     
- Complexity    32624    33656    +1032     
============================================
  Files          1877     1925      +48     
  Lines        139502   144951    +5449     
  Branches      15299    15849     +550     
============================================
+ Hits         102638   108068    +5430     
+ Misses        28908    28618     -290     
- Partials       7956     8265     +309     
Flag Coverage Δ
inttests 27.85% <10.81%> (+3.26%) ⬆️
systests 24.65% <10.81%> (+0.33%) ⬆️
unittests 73.90% <83.78%> (+1.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
.../apache/pulsar/broker/admin/impl/ClustersBase.java 81.76% <100.00%> (+1.26%) ⬆️
...r/common/policies/data/NamespaceIsolationData.java 100.00% <ø> (ø)
...mmon/policies/data/NamespaceIsolationDataImpl.java 89.18% <100.00%> (+3.89%) ⬆️
.../pulsar/admin/cli/CmdNamespaceIsolationPolicy.java 37.83% <0.00%> (+1.25%) ⬆️
...on/policies/impl/NamespaceIsolationPolicyImpl.java 84.90% <50.00%> (-1.37%) ⬇️
...cies/data/NamespaceIsolationPolicyUnloadScope.java 50.00% <50.00%> (ø)

... and 546 files with indirect coverage changes

Copy link
Member

@dao-jun dao-jun left a comment

Choose a reason for hiding this comment

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

LGTM, just a few minor comments.

@grssam
Copy link
Contributor Author

grssam commented Aug 29, 2024

PS: while cherry picking to branch-3.0 , it will contain one conflict due to the command line argument parser library change at line https://github.com/apache/pulsar/pull/23120/files#diff-63a2a931cc53a14767f5eb4a53898d7b6fb20f81a87de51cc15bbc139782bd00R77 (and the relevant class imports as well)

@lhotari
Copy link
Member

lhotari commented Aug 29, 2024

PS: while cherry picking to branch-3.0 , it will contain one conflict due to the command line argument parser library change at line https://github.com/apache/pulsar/pull/23120/files#diff-63a2a931cc53a14767f5eb4a53898d7b6fb20f81a87de51cc15bbc139782bd00R77 (and the relevant class imports as well)

@grssam Yes, that's common that there are conflicts. The committer performing the cherry-picking and backporting will request a separate PR for a backport to branch-3.0 if there's a need. Some of this is explained in the mailing list message https://lists.apache.org/thread/ryyfv8o33yyw9hmo5gnxn71boocn3mzs .

@grssam
Copy link
Contributor Author

grssam commented Aug 30, 2024

/pulsarbot rerun-failure-checks

@grssam grssam requested a review from pdolif August 30, 2024 12:38
@dao-jun
Copy link
Member

dao-jun commented Sep 1, 2024

merging...

@dao-jun dao-jun merged commit 8da3bf8 into apache:master Sep 1, 2024
51 checks passed
lhotari pushed a commit that referenced this pull request Sep 5, 2024
…icy set` call (#23120)

Co-authored-by: Zixuan Liu <[email protected]>
(cherry picked from commit 8da3bf8)
lhotari pushed a commit that referenced this pull request Sep 5, 2024
…icy set` call (#23120)

Co-authored-by: Zixuan Liu <[email protected]>
(cherry picked from commit 8da3bf8)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Sep 10, 2024
…icy set` call (apache#23120)

Co-authored-by: Zixuan Liu <[email protected]>
(cherry picked from commit 8da3bf8)
(cherry picked from commit d7af82e)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Sep 12, 2024
…icy set` call (apache#23120)

Co-authored-by: Zixuan Liu <[email protected]>
(cherry picked from commit 8da3bf8)
(cherry picked from commit d7af82e)
@lhotari lhotari added this to the 4.0.0 milestone Oct 14, 2024
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.

6 participants