[improve][admin,broker] Add option to unloadNamespaceBundle with bundle Affinity broker url #18663

vineeth1995 · 2022-11-28T21:25:42Z

Motivation

To provide an option for admin to unload namespace bundle with broker url to which the bundle has to be assigned to after the unload.

Modifications

Add an option to provide brokerWebServiceAddress (Ex: https://broker1.com:4443) while unloadingNamespaceBundle in the admin CLI and make corresponding changes in the pulsar-client-admin-api to support this.
Changes in pulsar-broker unloadNamespaceBundle API to accommodate broker url while unloading.
Add functionality in LoadManger implementation to select affinity broker for a bundle while trying to assign unloaded bundle to broker.

Verifying this change

This change added tests and can be verified as follows:

Added end to end test to make sure the bundle assignment happens as per the expectation

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

Matching PR in forked repository

PR in forked repository:

github-actions · 2022-11-28T21:26:52Z

@vineeth1995 Please select only one documentation label in your PR description.

codelipenghui · 2022-11-29T02:30:07Z

@vineeth1995 Thanks for the contribution.
The new feature should start with a proposal https://github.com/apache/pulsar/blob/master/wiki/proposals/PIP.md
So that we can understand the motivation for the API change.

codecov-commenter · 2022-11-29T18:41:54Z

Codecov Report

Merging #18663 (6f94be4) into master (ed33fb3) will increase coverage by 0.09%.
The diff coverage is 23.80%.

@@             Coverage Diff              @@
##             master   #18663      +/-   ##
============================================
+ Coverage     47.36%   47.46%   +0.09%     
- Complexity    10716    10733      +17     
============================================
  Files           712      712              
  Lines         69541    69598      +57     
  Branches       7466     7470       +4     
============================================
+ Hits          32941    33034      +93     
+ Misses        32894    32852      -42     
- Partials       3706     3712       +6

Flag	Coverage Δ
unittests	`47.46% <23.80%> (+0.09%)`	⬆️

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

Impacted Files	Coverage Δ
.../apache/pulsar/broker/loadbalance/LoadManager.java	`61.11% <ø> (ø)`
.../pulsar/broker/loadbalance/ModularLoadManager.java	`0.00% <ø> (ø)`
...che/pulsar/broker/loadbalance/NoopLoadManager.java	`0.00% <0.00%> (ø)`
...ache/pulsar/broker/namespace/NamespaceService.java	`58.08% <ø> (-0.15%)`	⬇️
...pache/pulsar/broker/admin/impl/NamespacesBase.java	`63.56% <2.94%> (-0.04%)`	⬇️
...broker/loadbalance/impl/SimpleLoadManagerImpl.java	`38.42% <20.00%> (-0.39%)`	⬇️
...roker/loadbalance/impl/ModularLoadManagerImpl.java	`63.06% <40.00%> (-6.07%)`	⬇️
...er/loadbalance/impl/ModularLoadManagerWrapper.java	`73.91% <70.00%> (-11.46%)`	⬇️
.../org/apache/pulsar/broker/admin/v1/Namespaces.java	`49.02% <100.00%> (+0.08%)`	⬆️
.../org/apache/pulsar/broker/admin/v2/Namespaces.java	`59.98% <100.00%> (+2.02%)`	⬆️
... and 53 more

rdhabalia

LGTM

codelipenghui

A proposal is required since the PR is changed the Admin API and introduced a new feature.

rdhabalia · 2022-11-30T02:11:31Z

@codelipenghui I don't think this will require PIP as it's part of adding a flag in admin API. There are a large number of PRs which must have gone through PIP with similar changes but merge without any explanation. let me give you few examples
#16167
#14680
#12136
#13938

also PR which breaks backward compatibility #10601 was merged without any concerns.
I have many such examples where I see practice by the specific community to discourage people from their contribution and delay their efforts. I would strongly discourage such destructive practices.
There are such bad practices going on in the community and I have many examples. Please let us know if you have any concerns with the PR by adding an option flag if not then you should not BLOCK the PR unnecessarily as it will start -ve and destructive practice in the community and we didn't open source Pulsar for such things.

rdhabalia · 2022-11-30T02:23:14Z

@codelipenghui I know there were multiple genuine PRs were blocked in past and they were sitting unmerged for a long time for multiple releases. I can clearly see the intention but we should avoid such destructive practice instead we should spend time in constructive efforts as not every contributor is vendor but users/companies are trying to contribute for their business usecases and such practice discourages companies to use Pulsar.

rdhabalia · 2022-11-30T20:37:33Z

@codelipenghui can you please unblock the PR?

rdhabalia · 2022-12-01T23:21:56Z

@codelipenghui ping

rdhabalia · 2022-12-02T09:01:31Z

this PR is blocked for last 3 days and I haven't heard anything from @codelipenghui . can we unblock the PR or dismiss the review? we can't keep following up on the same thing again and again.

rdhabalia · 2022-12-07T02:42:54Z

@codelipenghui waiting for your reply to remove the blocker? It's not acceptable to block the PR and not replying here.

codelipenghui · 2022-12-07T03:09:18Z

@codelipenghui I don't think this will require PIP as it's part of adding a flag in admin API. There are a large number of PRs which must have gone through PIP with similar changes but merge without any explanation. let me give you few examples
#16167
#14680
#12136
#13938

also PR which breaks backward compatibility #10601 was merged without any concerns.
I have many such examples where I see practice by the specific community to discourage people from their contribution and delay their efforts. I would strongly discourage such destructive practices.
There are such bad practices going on in the community and I have many examples. Please let us know if you have any concerns with the PR by adding an option flag if not then you should not BLOCK the PR unnecessarily as it will start -ve and destructive practice in the community and we didn't open source Pulsar for such things.

Sorry for the late response.

Yes, this is why I think it's better to have a proposal to avoid we will not introduce any API changes without a proposal in the future. I agree there are many changes to admin APIs without proposals/discussions. But we are on the wrong way, right? More and more people are starting to use Pulsar. We should be more careful about the API changes than ever.

I want to say there have been many CLI, and admin API changes without proposals before. If you just don't think we should not start with this PR(proposal required). It's ok for me. I think I should start a discussion under the mailing list first, not block this PR under an uncleared rule. I see all the PRs you provided are from StreamNative's engineers. Please don't think I mean to facilitate StreamNative engineers.

Here is an example of a proposal that is not required for new metrics.

#18319
#18560

There have been many CLI, and admin API changes without proposals before. It's better to have a discussion under the mailing list first.

Technoboy- · 2022-12-08T02:27:34Z

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

@@ -863,6 +866,50 @@ protected BookieAffinityGroupData internalGetBookieAffinityGroup() {
        }
    }

+    private void validateLeaderBroker() {


It's better to make this async

If we make it async, the result could be indeterministic. If the bundle-broker mapping is not set by the time loadmanager loads the bundle, it will unload the bundle onto wrong broker.

yes, it's dependent and we don't have to make it async here for this admin-api.

Technoboy- · 2022-12-08T02:29:09Z

...ar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/SimpleLoadManagerImpl.java

@@ -1443,6 +1447,15 @@ public void doNamespaceBundleSplit() throws Exception {
        }
    }

+    @Override
+    public String setNamespaceBundleAffinity(String bundle, String broker) {
+        if (broker == null) {


StringUtils.isBlank

Technoboy- · 2022-12-08T02:29:18Z

...r-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java

+
+    @Override
+    public String setNamespaceBundleAffinity(String bundle, String broker) {
+        if (broker == null) {


StringUtils.isBlank

Technoboy- · 2022-12-08T02:29:26Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/NoopLoadManager.java

@@ -142,4 +146,12 @@ public void stop() throws PulsarServerException {
        }
    }

+    @Override
+    public String setNamespaceBundleAffinity(String bundle, String broker) {
+        if (broker == null) {


StringUtils.isBlank

Technoboy- · 2022-12-08T02:35:28Z

...roker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerWrapper.java

@@ -146,4 +146,16 @@ public Set<String> getAvailableBrokers() throws Exception {
    public CompletableFuture<Set<String>> getAvailableBrokersAsync() {
        return loadManager.getAvailableBrokersAsync();
    }
+
+    private SimpleResourceUnit buildBrokerResourceUnit (String broker) {


One more blank here

codelipenghui

@vineeth1995 @rdhabalia

After reviewing all the changes, the PR introduced a new concept, bundle affinity, right? It's not only a new option for Admin API.

IMO, it should be 2 parts.

Namespace bundle affinity
Unload the namespace bundle with affinity (or we can just split it into 2 steps, set affinity, unload bundle)

And I don't see the bundleBrokerAffinityMap will be persistent somewhere. The bundle affinity will disappear after the broker restarts?

Sorry, I have to give a request changes here.

rdhabalia · 2022-12-14T08:47:10Z

And I don't see the bundleBrokerAffinityMap will be persistent somewhere. The bundle affinity will disappear after the broker restarts?

Yes, that's the purpose. we don't want persistent mapping it's useful when you want to debug by moving bundle temporarily to the specific broker. We already have namespace isolation policy for persistent affinity.

IMO, it should be 2 parts.

No, it's connected to each other where you want to unload bundle by setting specific broker affinity for temporary testing and you don't want to keep it persistent. Here, the main idea is to keep it non-persistent for quick testing, which is the main motivation to help users in debugging.

Sorry, I have to give a request changes here.

Let me know if you have any more concern so, we can remove the hold.

codelipenghui · 2022-12-19T03:42:25Z

@rdhabalia Thanks for the explanation. Clear for me now.

eolivelli

Very good
I generally support this work.
I left one minor comment that is worth to be addressed

eolivelli · 2022-12-19T07:34:05Z

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

+                log.error("The leader broker url is malformed - {}", leaderBrokerUrl);
+                throw new RestException(exception);
+            } catch (ExecutionException | InterruptedException exception) {
+                log.error("Leader broker not found - {}", leaderBrokerUrl);


ExecutionException may mean many things,
We should unwrap the cause

eolivelli · 2022-12-19T07:37:38Z

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

+            CompletableFuture<LookupResult> result = pulsar().getNamespaceService()
+                    .createLookupResult(leaderBrokerUrl, false, null);
+            try {
+                LookupResult lookupResult = result.get();


We should always set a timeout, otherwise the thread may be stuck forever

… bundle Affinity broker url

…bundle Affinity broker url

…le Affinity broker url

vineeth1995 · 2022-12-22T03:06:23Z

@eolivelli I addressed your comments.

rdhabalia · 2022-12-30T20:09:00Z

@eolivelli can you remove the blocker?

eolivelli · 2022-12-31T10:38:41Z

Lgtm

Please rebase.
We moved the website to another repository, can you please send the docs update to the website repository please? @tisonkun maybe can help

tisonkun · 2022-12-31T10:50:32Z

Conflict resolved in 6f94be4
Counterpart in the site repo in [improve][admin] unloadNamespaceBundle now accepts destinationBroker param pulsar-site#353

Since this is the first case, I made it a demo to be referred to later. Hopefully, we don't have new cases, but we only need to clear the backlog.

tisonkun · 2022-12-31T12:29:10Z

Merging...

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 28, 2022

vineeth1995 changed the title ~~Bundle unload~~ [improve][admin,broker] Add option to unloadNamespaceBundle with bundle Affinity broker url Nov 28, 2022

github-actions bot added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-not-needed Your PR changes do not impact docs labels Nov 28, 2022

codelipenghui added the wants/proposal label Nov 29, 2022

rdhabalia assigned vineeth1995 Nov 29, 2022

rdhabalia added this to the 2.11.0 milestone Nov 29, 2022

rdhabalia added the ready-to-test label Nov 29, 2022

rdhabalia approved these changes Nov 29, 2022

View reviewed changes

codelipenghui previously requested changes Nov 30, 2022

View reviewed changes

tisonkun removed the wants/proposal label Dec 6, 2022

Technoboy- reviewed Dec 8, 2022

View reviewed changes

codelipenghui requested changes Dec 8, 2022

View reviewed changes

vineeth1995 force-pushed the bundleUnload branch from b17fcc4 to 85800be Compare December 14, 2022 23:20

codelipenghui approved these changes Dec 19, 2022

View reviewed changes

eolivelli requested changes Dec 19, 2022

View reviewed changes

eolivelli reviewed Dec 19, 2022

View reviewed changes

Vineeth added 13 commits December 20, 2022 22:04

[improve][pulsar-broker] Provide option to unloadNamespaceBundle with…

10a866b

… bundle Affinity broker url

[improve][admin,broker] Provide option to unloadNamespaceBundle with …

0a7b068

…bundle Affinity broker url

[improve][admin,broker] Add option to unloadNamespaceBundle with bund…

e43fc50

…le Affinity broker url

[improve][admin,broker] Add option to unloadNamespaceBundle with bund…

1d2a971

…le Affinity broker url

[improve][admin,broker] Add option to unloadNamespaceBundle with bund…

94def53

…le Affinity broker url

[improve][admin,broker] Add option to unloadNamespaceBundle with bund…

ef8013a

…le Affinity broker url

[improve][admin,broker] Add option to unloadNamespaceBundle with bund…

ab7130b

…le Affinity broker url

[improve][admin,broker] Add option to unloadNamespaceBundle with bund…

d660951

…le Affinity broker url

[improve][admin,broker] Add option to unloadNamespaceBundle with bund…

a75ff44

…le Affinity broker url

[improve][admin,broker] Add option to unloadNamespaceBundle with bund…

09d4f76

…le Affinity broker url

[improve][admin,broker] Add option to unloadNamespaceBundle with bund…

f2b5759

…le Affinity broker url

[improve][admin,broker] Add option to unloadNamespaceBundle with bund…

fc9be0a

…le Affinity broker url

[improve][admin,broker] Add option to unloadNamespaceBundle with bund…

e86bdb1

…le Affinity broker url

vineeth1995 force-pushed the bundleUnload branch from 85800be to e86bdb1 Compare December 21, 2022 23:06

rdhabalia requested a review from eolivelli December 25, 2022 06:20

eolivelli approved these changes Dec 31, 2022

View reviewed changes

tisonkun mentioned this pull request Dec 31, 2022

[improve][admin] unloadNamespaceBundle now accepts destinationBroker param apache/pulsar-site#353

Merged

Merge branch 'master' into bundleUnload

6f94be4

tisonkun merged commit e194c01 into apache:master Dec 31, 2022

codelipenghui mentioned this pull request Feb 13, 2023

PIP-192: New Pulsar Broker Load Balancer #16691

Closed

Demogorgon314 mentioned this pull request Jul 19, 2023

[fix][broker] Avoid infinite bundle unloading #20822

Merged

4 tasks

[improve][admin,broker] Add option to unloadNamespaceBundle with bundle Affinity broker url #18663

[improve][admin,broker] Add option to unloadNamespaceBundle with bundle Affinity broker url #18663

Conversation

vineeth1995 commented Nov 28, 2022 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

github-actions bot commented Nov 28, 2022

codelipenghui commented Nov 29, 2022

codecov-commenter commented Nov 29, 2022 • edited Loading

Codecov Report

rdhabalia left a comment

Choose a reason for hiding this comment

codelipenghui left a comment

Choose a reason for hiding this comment

rdhabalia commented Nov 30, 2022

rdhabalia commented Nov 30, 2022

rdhabalia commented Nov 30, 2022

rdhabalia commented Dec 1, 2022

rdhabalia commented Dec 2, 2022

rdhabalia commented Dec 7, 2022

codelipenghui commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codelipenghui left a comment

Choose a reason for hiding this comment

rdhabalia commented Dec 14, 2022

codelipenghui commented Dec 19, 2022

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vineeth1995 commented Dec 22, 2022

rdhabalia commented Dec 30, 2022

eolivelli commented Dec 31, 2022

tisonkun commented Dec 31, 2022

tisonkun commented Dec 31, 2022

vineeth1995 commented Nov 28, 2022 •

edited

Loading

codecov-commenter commented Nov 29, 2022 •

edited

Loading