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

grssam · 2024-08-02T12:52:49Z

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

Documentation

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

PR in forked repo - grssam#1

github-actions · 2024-08-02T12:53:20Z

@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 -->

grssam · 2024-08-22T07:17:08Z

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

lhotari

LGTM. Outstanding work @grssam! Thanks for contributing.

grssam · 2024-08-28T07:56:40Z

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 · 2024-08-28T10:45:34Z

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 · 2024-08-28T10:58:52Z

/pulsarbot rerun-failure-checks

grssam · 2024-08-28T11:24:45Z

/pulsarbot rerun-failure-checks

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java

grssam · 2024-08-29T11:55:11Z

/pulsarbot rerun-failure-checks

codecov-commenter · 2024-08-29T12:21:36Z

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 554 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

@@             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

dao-jun

LGTM, just a few minor comments.

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApi2Test.java

grssam · 2024-08-29T13:20:52Z

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 · 2024-08-29T13:57:15Z

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 .

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java

…l/ClustersBase.java Co-authored-by: Zixuan Liu <[email protected]>

grssam · 2024-08-30T12:35:26Z

/pulsarbot rerun-failure-checks

dao-jun · 2024-09-01T04:19:02Z

merging...

…icy set` call (#23120) Co-authored-by: Zixuan Liu <[email protected]> (cherry picked from commit 8da3bf8)

…icy set` call (apache#23120) Co-authored-by: Zixuan Liu <[email protected]> (cherry picked from commit 8da3bf8) (cherry picked from commit d7af82e)

[WIP] Introduce unload flag in ns-isolation-policy set call

4d6fce9

github-actions bot added the doc-label-missing label Aug 2, 2024

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 added 4 commits August 12, 2024 16:03

Changes based on PIP review

8384b27

checkstyle fixes

976bc16

minor comment name fix

e467025

Replace Pattern with String for sets

55c5e75

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 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 marked this pull request as ready for review August 19, 2024 06:06

lhotari added release/3.0.7 release/3.3.2 labels Aug 22, 2024

grssam added 2 commits August 23, 2024 20:50

Test correct namespaces are unloaded based on the scope

7c8ab79

remove whitespace

61e0200

lhotari requested review from poorbarcode and lhotari August 23, 2024 16:09

lhotari approved these changes Aug 23, 2024

View reviewed changes

lhotari requested review from rdhabalia, AnonHxy, equanz, yuruguo, eolivelli, Technoboy- and RobertIndie August 23, 2024 17:45

nodece self-requested a review August 29, 2024 08:51

phil-cd reviewed Aug 29, 2024

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java Outdated Show resolved Hide resolved

typo fix in log line

a799493

dao-jun approved these changes Aug 29, 2024

View reviewed changes

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApi2Test.java Show resolved Hide resolved

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApi2Test.java Show resolved Hide resolved

nodece reviewed Aug 30, 2024

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java Outdated Show resolved Hide resolved

Update pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/imp…

e1372da

…l/ClustersBase.java Co-authored-by: Zixuan Liu <[email protected]>

nodece approved these changes Aug 30, 2024

View reviewed changes

Add test for primary broker changed case

746f072

lhotari approved these changes Aug 30, 2024

View reviewed changes

grssam requested a review from phil-cd August 30, 2024 12:38

dao-jun merged commit 8da3bf8 into apache:master Sep 1, 2024
51 checks passed

iosdev747 mentioned this pull request Sep 2, 2024

[Bug] Breaking contract b/w 2.9 / 2.10 and 3.0 for ns-isolation-policy #23092

Closed

3 tasks

grssam mentioned this pull request Sep 4, 2024

[improve][admin] PIP-369 Change default value of unload-scope in ns-isolation-policy set #23253

Open

15 tasks

lhotari pushed a commit that referenced this pull request Sep 5, 2024

[improve][admin] PIP-369 Introduce unload flag in `ns-isolation-pol…

d7af82e

…icy set` call (#23120) Co-authored-by: Zixuan Liu <[email protected]> (cherry picked from commit 8da3bf8)

lhotari added the cherry-picked/branch-3.0 label Sep 5, 2024

lhotari pushed a commit that referenced this pull request Sep 5, 2024

[improve][admin] PIP-369 Introduce unload flag in `ns-isolation-pol…

f9283da

…icy set` call (#23120) Co-authored-by: Zixuan Liu <[email protected]> (cherry picked from commit 8da3bf8)

lhotari added the cherry-picked/branch-3.3 label Sep 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

grssam commented Aug 2, 2024 •

edited

Loading

github-actions bot commented Aug 2, 2024

grssam commented Aug 22, 2024

lhotari left a comment

grssam commented Aug 28, 2024

lhotari commented Aug 28, 2024 •

edited

Loading

grssam commented Aug 28, 2024

grssam commented Aug 28, 2024

grssam commented Aug 29, 2024

codecov-commenter commented Aug 29, 2024 •

edited

Loading

dao-jun left a comment

grssam commented Aug 29, 2024 •

edited

Loading

lhotari commented Aug 29, 2024

grssam commented Aug 30, 2024

dao-jun commented Sep 1, 2024

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

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

Conversation

grssam commented Aug 2, 2024 • edited Loading

Modifications

Verifying this change

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

Documentation

github-actions bot commented Aug 2, 2024

grssam commented Aug 22, 2024

lhotari left a comment

Choose a reason for hiding this comment

grssam commented Aug 28, 2024

lhotari commented Aug 28, 2024 • edited Loading

grssam commented Aug 28, 2024

grssam commented Aug 28, 2024

grssam commented Aug 29, 2024

codecov-commenter commented Aug 29, 2024 • edited Loading

Codecov Report

dao-jun left a comment

Choose a reason for hiding this comment

grssam commented Aug 29, 2024 • edited Loading

lhotari commented Aug 29, 2024

grssam commented Aug 30, 2024

dao-jun commented Sep 1, 2024

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

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

grssam commented Aug 2, 2024 •

edited

Loading

lhotari commented Aug 28, 2024 •

edited

Loading

codecov-commenter commented Aug 29, 2024 •

edited

Loading

grssam commented Aug 29, 2024 •

edited

Loading