[improve][pip] PIP-369: Flag based selective unload on changing ns-isolation-policy

[iosdev747]

Discussion thread: #23094

Motivation

In Apache Pulsar 3.x, changing a namespace isolation policy results in unloading all namespace bundles that match the namespace's regular expression provided in the isolation policy.
This can be problematic for cases where the regex matches a large subset of namespaces, such as tenant-x/.*.
One of such case is mentioned on this issue #23092 where policy change resulted in 100+ namespace bundles to get unloaded.
And broker exhausted all the available connections due to too many unload calls happening at once resulting in 5xx response.
Other issues that happens with this approach are huge latency spikes as topics are unavailable until bundles are loaded back, increasing the pending produce calls.
The only benefit this approach serves is ensuring that all the namespaces matching the policy regex will come to correct broker group.
But when namespace bundles are already on the correct broker group (according to the policy), unloading those namespaces doesn't serve any purpose.

This PIP aims to address the need to either prevent unnecessary unloading or provide a more granular approach to determine what should be unloaded.

Some of the cases covered by this PIP are discussed in #23094 by @grssam.

• unload nothing as part of the set policy call
• unload every matching namespace as part of the policy set call
• unload only the changed namespaces (newly added + removed)

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

Matching PR in forked repository

PR in forked repository:

[github-actions]

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

[lhotari]

Well written and explained. Thanks.

In the initial review, I only have a few knits about improving some concepts such as the unload enum in the proposal.
I think that putting more effort in the naming of it and the different values could be useful. For example, all should be something like MATCHING and the name of the enum should be more descriptive.

[grssam]

Well written and explained. Thanks.

In the initial review, I only have a few knits about improving some concepts such as the unload enum in the proposal. I think that putting more effort in the naming of it and the different values could be useful. For example, all should be something like MATCHING and the name of the enum should be more descriptive.

Thanks for the feedback. We can certainly name it all_matching . Notice that I am still using snake case to be consistent with some other enums used in the same policy call.
Generally the description of a flag is meant to contain more context. It's very tricky to convey what this param would do without exploding the param length.. How about something like unload-scope does that make more sense at first glance?

[lhotari]

Thanks for the feedback. We can certainly name it all_matching . Notice that I am still using snake case to be consistent with some other enums used in the same policy call.
Generally the description of a flag is meant to contain more context. It's very tricky to convey what this param would do without exploding the param length.. How about something like unload-scope does that make more sense at first glance?

+1. Sounds good! UnloadScope would be a great concise name for the enum. Snake case values are fine if that's already the convention.

[iosdev747]

Thank you everyone for giving the feedback :)
I have addressed the review comments. And adding explicitly in the PIP that this should get cherry picked in 3.0.x as it provides a way to address the issue mentioned in #23092.

For changing the definition of all/all_matching and changing the default option to changed, it can be taken in a separate PR that will be present in 4.0+ but not in 3.0.x

I'll start a voting thread.

[lhotari]

minor detail: unload_scope (snake case) as the enum name is perhaps against typical conventions. I believe that most enums are CamelCase, so instead of unload_scope, the enum type name should be UnloadScope.