pulsar icon indicating copy to clipboard operation
pulsar copied to clipboard

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

Open grssam opened this issue 1 year ago • 6 comments
trafficstars

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
  • [x] The admin CLI options
  • [ ] The metrics
  • [ ] Anything that affects deployment

Documentation

  • [ ] doc
  • [x] doc-required
  • [ ] doc-not-needed
  • [ ] doc-complete

PR in forked repo - https://github.com/grssam/pulsar/pull/1

grssam avatar Aug 02 '24 12:08 grssam

@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[bot] avatar Aug 02 '24 12:08 github-actions[bot]

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

grssam avatar Aug 22 '24 07:08 grssam

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

grssam avatar Aug 28 '24 07:08 grssam

Not able to rerun the failed CI pipeline, but all tests passed in my repo - https://github.com/grssam/pulsar/pull/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.

lhotari avatar Aug 28 '24 10:08 lhotari

/pulsarbot rerun-failure-checks

grssam avatar Aug 28 '24 10:08 grssam

/pulsarbot rerun-failure-checks

grssam avatar Aug 28 '24 11:08 grssam

/pulsarbot rerun-failure-checks

grssam avatar Aug 29 '24 11:08 grssam

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 :warning:
.../pulsar/admin/cli/CmdNamespaceIsolationPolicy.java 0.00% 1 Missing :warning:
...on/policies/impl/NamespaceIsolationPolicyImpl.java 50.00% 1 Missing :warning:
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%) :arrow_up:
systests 24.65% <10.81%> (+0.33%) :arrow_up:
unittests 73.90% <83.78%> (+1.05%) :arrow_up:

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%) :arrow_up:
...r/common/policies/data/NamespaceIsolationData.java 100.00% <ø> (ø)
...mmon/policies/data/NamespaceIsolationDataImpl.java 89.18% <100.00%> (+3.89%) :arrow_up:
.../pulsar/admin/cli/CmdNamespaceIsolationPolicy.java 37.83% <0.00%> (+1.25%) :arrow_up:
...on/policies/impl/NamespaceIsolationPolicyImpl.java 84.90% <50.00%> (-1.37%) :arrow_down:
...cies/data/NamespaceIsolationPolicyUnloadScope.java 50.00% <50.00%> (ø)

... and 546 files with indirect coverage changes

codecov-commenter avatar Aug 29 '24 12:08 codecov-commenter

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 avatar Aug 29 '24 13:08 grssam

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 .

lhotari avatar Aug 29 '24 13:08 lhotari

/pulsarbot rerun-failure-checks

grssam avatar Aug 30 '24 12:08 grssam

merging...

dao-jun avatar Sep 01 '24 04:09 dao-jun