PIP-61: Advertise multiple addresses #6903

zplinuxlover · 2020-05-07T01:48:45Z

this code change is for
https://github.com/apache/pulsar/wiki/PIP-61%3A-Advertised-multiple-addresses

1. resolve broker.conf, validate `advertisedListeners` and `internalListenerName` 2. register the `advertisedListeners` to zookeeper

1. resolve broker.conf, validate `advertisedListeners` and `internalListenerName` 2. register the `advertisedListeners` to zookeeper 3. client find the target broker with listenerName

1. resolve broker.conf, validate `advertisedListeners` and `internalListenerName` 2. register the `advertisedListeners` to zookeeper 3. client find the target broker with listenerName 4. add test case PulsarMultiListenersTest

1. resolve broker.conf, validate `advertisedListeners` and `internalListenerName` 2. register the `advertisedListeners` to zookeeper 3. client find the target broker with listenerName 4. add test case PulsarMultiListenersTest 5. add test case MultipleListenerValidatorTest

1. resolve broker.conf, validate `advertisedListeners` and `internalListenerName` 2. register the `advertisedListeners` to zookeeper 3. client find the target broker with listenerName 4. add test case PulsarMultiListenersTest 5. add test case MultipleListenerValidatorTest 6. remove necessary import

1. resolve broker.conf, validate `advertisedListeners` and `internalListenerName` 2. register the `advertisedListeners` to zookeeper 3. client find the target broker with listenerName 4. add test case PulsarMultiListenersTest 5. add test case MultipleListenerValidatorTest 6. remove necessary import 7. add license header

zplinuxlover · 2020-05-07T02:16:57Z

/pulsarbot run-failure-checks

1. resolve broker.conf, validate `advertisedListeners` and `internalListenerName` 2. register the `advertisedListeners` to zookeeper 3. client find the target broker with listenerName 4. add test case PulsarMultiListenersTest 5. add test case MultipleListenerValidatorTest 6. remove necessary import 7. add license header

zplinuxlover · 2020-05-07T06:11:57Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-05-07T06:12:02Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-05-07T06:12:19Z

/pulsarbot run-failure-checks

sijie

The change looks good in general. Left a couple of comments.

sijie · 2020-05-18T22:03:13Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

@@ -146,6 +146,14 @@
    )
    private String advertisedAddress;

+    //
+    @FieldContext(category=CATEGORY_SERVER, doc = "")


Can you add documentation for these two settings?

Also, can you add these settings to broker.conf file?

I have add the configuration doc here

sijie · 2020-05-18T22:03:30Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

@@ -146,6 +146,14 @@
    )
    private String advertisedAddress;

+    //


nit: remove this line if it is not needed.

I have remove the code for this line

sijie · 2020-05-18T22:34:35Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceEphemeralData.java

+    @NotNull
+    public Map<String, AdvertisedListener> getAdvertisedListeners() {
+        if (this.advertisedListeners == null) {
+            return Collections.unmodifiableMap(Collections.EMPTY_MAP);


nit: I think EMPTY_MAP is already unmodifiable.

I have fix the code

sijie · 2020-05-18T22:39:20Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java

+                            return;
+                        } else {
+                            future.complete(Optional.of(new LookupResult(
+                                    nsData.get().getHttpUrl(), nsData.get().getHttpUrlTls(), listener.getBrokerServiceUrl().toString(),


Why do we get HttpUrl and HttpUrlTls from NamespaceBundle but get brokerServiceUrl and brokerServiceUrlTls?

currently we only concern pulsar protocol here, the HttpUrl and HttpUrlTls behavior as before

sijie · 2020-05-18T22:39:38Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java

+                                return;
+                            } else {
+                                lookupFuture.complete(Optional.of(new LookupResult(
+                                        ownerInfo.getHttpUrl(), ownerInfo.getHttpUrlTls(), listener.getBrokerServiceUrl().toString(),


same comment as above

currently we only concern pulsar protocol here, the HttpUrl and HttpUrlTls behavior as before.

sijie · 2020-05-18T22:40:58Z

...ar-common/src/main/java/org/apache/pulsar/policies/data/loadbalancer/AdvertisedListener.java

+@Builder
+@NoArgsConstructor
+@AllArgsConstructor
+public class AdvertisedListener {


Why do we only include brokerServiceUrl and brokerServiceUrlTls in AdvertisedListener?

currently we only concern pulsar protocol here, the HttpUrl and HttpUrlTls behavior as before

zplinuxlover · 2020-05-20T07:41:24Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-05-20T09:19:18Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-05-21T06:03:03Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-05-21T08:23:41Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-05-22T02:43:39Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-05-22T13:42:27Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-05-22T22:52:58Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-05-22T23:56:03Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-05-23T01:00:57Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-05-23T01:01:15Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-05-23T01:01:43Z

/pulsarbot run-failure-checks

codelipenghui

Looks good to me, just some left some minor comments.

codelipenghui · 2020-05-25T02:23:16Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

-import java.util.Map;
-import java.util.Objects;
-import java.util.Optional;
+import java.util.*;


Avoid use import .*

resolve the problem

codelipenghui · 2020-05-25T02:30:54Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java

+     * Lookup broker-service address for a given namespace-bundle which contains given topic.
+     *
+     * a. Returns broker-address if namespace-bundle is already owned by any broker b. If current-broker receives
+     * lookup-request and if it's not a leader then current broker redirects request to leader by returning
+     * leader-service address. c. If current-broker is leader then it finds out least-loaded broker to own namespace
+     * bundle and redirects request by returning least-loaded broker. d. If current-broker receives request to own the
+     * namespace-bundle then it owns a bundle and returns success(connect) response to client.


It's better to use owner rather than leader.
And, for easier reading. Please separate a b c d into different paragraphs.

I have format the comment

1. remove java.util.* code format 2. format function comment

zplinuxlover · 2020-05-25T07:01:17Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-05-25T07:52:11Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-05-25T09:52:49Z

/pulsarbot run-failure-checks

codelipenghui · 2020-06-02T02:53:06Z

ping @sijie please help review this PR again.

zplinuxlover · 2020-06-02T05:58:19Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-06-02T07:48:46Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-06-02T09:53:05Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-06-03T01:33:45Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-06-03T01:34:44Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-06-03T03:17:30Z

/pulsarbot run-failure-checks

zplinuxlover · 2020-06-03T06:19:16Z

/pulsarbot run-failure-checks

* PIP-61: 1. resolve broker.conf, validate `advertisedListeners` and `internalListenerName` 2. register the `advertisedListeners` to zookeeper 3. client find the target broker with listenerName 4. add test case PulsarMultiListenersTest 5. add test case MultipleListenerValidatorTest

first file added 7 commits April 24, 2020 14:30

PIP-61:

be868bb

1. resolve broker.conf, validate `advertisedListeners` and `internalListenerName` 2. register the `advertisedListeners` to zookeeper

PIP-61:

6303ad4

1. resolve broker.conf, validate `advertisedListeners` and `internalListenerName` 2. register the `advertisedListeners` to zookeeper 3. client find the target broker with listenerName

PIP-61:

9dbcebf

1. resolve broker.conf, validate `advertisedListeners` and `internalListenerName` 2. register the `advertisedListeners` to zookeeper 3. client find the target broker with listenerName 4. add test case PulsarMultiListenersTest

PIP-61:

0789052

1. resolve broker.conf, validate `advertisedListeners` and `internalListenerName` 2. register the `advertisedListeners` to zookeeper 3. client find the target broker with listenerName 4. add test case PulsarMultiListenersTest 5. add test case MultipleListenerValidatorTest

Merge branch 'master' into feature/pip-61

25df9e9

PIP-61:

9fa3fde

1. resolve broker.conf, validate `advertisedListeners` and `internalListenerName` 2. register the `advertisedListeners` to zookeeper 3. client find the target broker with listenerName 4. add test case PulsarMultiListenersTest 5. add test case MultipleListenerValidatorTest

codelipenghui requested review from sijie, merlimat and jiazhai May 7, 2020 02:01

codelipenghui assigned zplinuxlover May 7, 2020

codelipenghui added this to the 2.6.0 milestone May 7, 2020

codelipenghui added the type/feature The PR added a new feature or issue requested a new feature label May 7, 2020

first file added 5 commits May 7, 2020 10:33

sijie changed the title ~~Feature/pip 61~~ PIP-61: Advertise multiple addresses May 7, 2020

zplinuxlover removed their assignment May 7, 2020

sijie reviewed May 18, 2020

View reviewed changes

fix code by review

c39a06d

sijie mentioned this pull request May 20, 2020

[discussion] Pulsar release 2.6.0 #5819

Closed

Merge remote-tracking branch 'origin/master' into feature/pip-61

f381c69

codelipenghui assigned zplinuxlover May 25, 2020

codelipenghui approved these changes May 25, 2020

View reviewed changes

fix code by review

159ec9d

1. remove java.util.* code format 2. format function comment

zplinuxlover requested a review from sijie May 29, 2020 09:23

codelipenghui and others added 2 commits June 2, 2020 10:54

Merge branch 'master' into feature/pip-61

5bfdc16

remove unused import in Commands.java file.

57100be

codelipenghui merged commit 8e40bd9 into apache:master Jun 3, 2020

codelipenghui mentioned this pull request Jun 4, 2020

Advertised multiple addresses for brokers #6252

Closed

PIP-61: Advertise multiple addresses #6903

PIP-61: Advertise multiple addresses #6903

Conversation

zplinuxlover commented May 7, 2020

zplinuxlover commented May 7, 2020

zplinuxlover commented May 7, 2020

zplinuxlover commented May 7, 2020

zplinuxlover commented May 7, 2020

sijie 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

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

zplinuxlover commented May 20, 2020

zplinuxlover commented May 20, 2020

zplinuxlover commented May 21, 2020

zplinuxlover commented May 21, 2020

zplinuxlover commented May 22, 2020

zplinuxlover commented May 22, 2020

zplinuxlover commented May 22, 2020

zplinuxlover commented May 22, 2020

zplinuxlover commented May 23, 2020

zplinuxlover commented May 23, 2020

zplinuxlover commented May 23, 2020

codelipenghui 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

zplinuxlover commented May 25, 2020

zplinuxlover commented May 25, 2020

zplinuxlover commented May 25, 2020

codelipenghui commented Jun 2, 2020

zplinuxlover commented Jun 2, 2020

zplinuxlover commented Jun 2, 2020

zplinuxlover commented Jun 2, 2020

zplinuxlover commented Jun 3, 2020

zplinuxlover commented Jun 3, 2020

zplinuxlover commented Jun 3, 2020

zplinuxlover commented Jun 3, 2020