[improve][admin,broker] Add option to unloadNamespaceBundle with bundle Affinity broker url by vineeth1995 · Pull Request #7 · vineeth1995/pulsar

vineeth1995 · 2022-11-07T20:10:58Z

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

rdhabalia

LGTM . minor comments.

rdhabalia · 2022-11-08T21:20:40Z

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

+        if (destinationBroker != null) {
+            if (!this.isLeaderBroker()) {
+                LeaderBroker leaderBroker = pulsar().getLeaderElectionService().getCurrentLeader().get();
+                String leaderBrokerUrl = leaderBroker.getServiceUrl();


this will not work in case of https. we need to select URL based on the request type HTTP or HTTPS?
you can find reference at: https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java#L104

This will work based on incoming requests ( http or https).

rdhabalia · 2022-11-08T21:22:43Z

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

+                String leaderBrokerUrl = leaderBroker.getServiceUrl();
+                try {
+                    URL redirectUrl = new URL(leaderBrokerUrl);
+                    URI redirect = UriBuilder.fromUri(uri.getRequestUri()).host(redirectUrl.getHost())


also, it's better to separate out this logic in a separate method
eg: validateLeader(httpRequest) this sends redirect response by throwing temporaryRedirect-Exception if current broker is not leader.

rdhabalia · 2022-11-08T21:23:11Z

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

+                                    false).build();
+
+                    // Redirect
+                    log.debug("Redirecting the rest call to leader - {}, bundleRange - {}", redirect, bundleRange);


always try to keep log.debug under if -> if(log.isDebugEnabled)

rdhabalia · 2022-11-08T21:24:05Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/Namespaces.java

            @PathParam("namespace") String namespace, @PathParam("bundle") String bundleRange,
-            @QueryParam("authoritative") @DefaultValue("false") boolean authoritative) {
+            @QueryParam("authoritative") @DefaultValue("false") boolean authoritative,
+            @QueryParam("destinationBroker") String destinationBroker) {


we need to update the doc in admin-api-topics.md file

rdhabalia · 2022-11-08T21:25:08Z

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

    }

+    public void setNamespaceBundleAffinity (String bundleRange, String destinationBroker) {
+        if (destinationBroker != null) {


try to avoid nested if cyclomatic complexity. instead do

if (StringUtils.isBlank(destinationBroker)) { return; }

rdhabalia · 2022-11-08T21:26:49Z

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

+
+    void setNamespaceBundleAffinity(String bundle, String broker);
+
+    String removeNamespaceBundleAffinity(String bundle);


ideally, if we have set api then we can avoid delete API. do we really need delete API? can we just call set with null value for removing affinity.

removeNamespaceBundleAffinity also serves the purpose of get heree

I mean instead of calling remove, can we just call set(bundle, null) -> null broker means remove affinity.?

rdhabalia · 2022-11-08T21:30:02Z

test-case failure

 Error:  Tests run: 7, Failures: 1, Errors: 0, Skipped: 6, Time elapsed: 31.675 s <<< FAILURE! - in org.apache.pulsar.broker.stats.MetadataStoreStatsTest
  Error:  testBatchMetadataStoreMetrics(org.apache.pulsar.broker.stats.MetadataStoreStatsTest)  Time elapsed: 10.762 s  <<< FAILURE!
  java.lang.AssertionError: expected object to not be null
  	at org.testng.Assert.fail(Assert.java:99)
  	at org.testng.Assert.assertNotNull(Assert.java:942)
  	at org.testng.Assert.assertNotNull(Assert.java:926)
  	at org.apache.pulsar.broker.stats.MetadataStoreStatsTest.testBatchMetadataStoreMetrics(MetadataStoreStatsTest.java:185)

can you verify if it's failing for you as well

vineeth1995 · 2022-11-14T21:48:29Z

test-case failure

 Error:  Tests run: 7, Failures: 1, Errors: 0, Skipped: 6, Time elapsed: 31.675 s <<< FAILURE! - in org.apache.pulsar.broker.stats.MetadataStoreStatsTest
  Error:  testBatchMetadataStoreMetrics(org.apache.pulsar.broker.stats.MetadataStoreStatsTest)  Time elapsed: 10.762 s  <<< FAILURE!
  java.lang.AssertionError: expected object to not be null
  	at org.testng.Assert.fail(Assert.java:99)
  	at org.testng.Assert.assertNotNull(Assert.java:942)
  	at org.testng.Assert.assertNotNull(Assert.java:926)
  	at org.apache.pulsar.broker.stats.MetadataStoreStatsTest.testBatchMetadataStoreMetrics(MetadataStoreStatsTest.java:185)

can you verify if it's failing for you as well

Yes, it passes for me in the local

rdhabalia · 2022-11-14T21:50:44Z

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

+            String leaderBrokerUrl = leaderBroker.getServiceUrl();
+            try {
+                URL redirectUrl = new URL(leaderBrokerUrl);
+                URI redirect = UriBuilder.fromUri(uri.getRequestUri()).host(redirectUrl.getHost())


it always returns http?

shouldn't it be based on http-request that comes from user . I guess I gave code reference earlier?

here uri is the input request uri. So if the input request has https, uri. getRequestUri() returns https url and we are only replacing host and port. The same is being used here.

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java

Line 498 in 27186a1

return UriBuilder.fromUri(uri.getRequestUri()).host(webUrl.getHost()).port(webUrl.getPort()).build();

rdhabalia

LGTM. minor comments. you can address them and can create PR in open source.

rdhabalia · 2022-11-28T20:45:36Z

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

+
+                // Redirect
+                if (log.isDebugEnabled()) {
+                    log.debug("Redirecting the rest call to leader - {}", redirect);


Redirecting the request call to leader

rdhabalia · 2022-11-28T20:49:31Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java

                                            .port(webUrl.get().getPort()).replaceQueryParam("authoritative",
-                                                    newAuthoritative).build();
-
+                                                    newAuthoritative).replaceQueryParam("destinationBroker",


if destinationBroker is optional then if we don't pass it then by default value will be null. so, is it necessary to pass null value for destinationBroker ? or can we just make any change here if we are any passing null value?

We are replacing the param with null because if destination broker is not null in the input request, it will again try to call leader broker to add the input to bundle affinity map which is already done before this step.

rdhabalia · 2022-11-28T20:57:06Z

pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java

                getAdmin().namespaces().unload(namespace);
            } else {
-                getAdmin().namespaces().unloadNamespaceBundle(namespace, bundle);
+                if (destinationBroker == null) {


we should avoid if/else here. what if we just call unloadNamespaceBundle with null destination. As it's passed as a query-param, server is anyway ignoring the value of destinationBroker if it has null value. right?

getAdmin().namespaces().unloadNamespaceBundle(namespace, bundle, destinationBroker);

… bundle Affinity broker url

…bundle Affinity broker url

…le Affinity broker url

github-actions · 2023-01-31T02:33:49Z

The pr had no activity for 30 days, mark with Stale label.

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

vineeth1995 force-pushed the bundleUnload branch from feb9f76 to d601cb7 Compare November 8, 2022 15:59

rdhabalia reviewed Nov 8, 2022

View reviewed changes

rdhabalia reviewed Nov 14, 2022

View reviewed changes

rdhabalia reviewed Nov 28, 2022

View reviewed changes

vineeth1995 force-pushed the bundleUnload branch from 52794a3 to 39a9810 Compare November 28, 2022 21:24

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

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

Merge branch 'master' into bundleUnload

6f94be4

github-actions bot added the Stale label Jan 31, 2023


		void setNamespaceBundleAffinity(String bundle, String broker);

		String removeNamespaceBundleAffinity(String bundle);

Conversation

vineeth1995 commented Nov 7, 2022

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

Uh oh!

rdhabalia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdhabalia commented Nov 8, 2022

Uh oh!

vineeth1995 commented Nov 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vineeth1995 Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdhabalia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vineeth1995 Nov 15, 2022 •

edited

Loading