Support multiple listeners without configuring broker's advertisedListeners #864

BewareMyPower · 2021-10-28T07:42:32Z

Fixes #818

Motivation

#742 adds the multiple listeners support for KoP 2.8.x. However, there're some flaws.

First, the KoP advertised listeners must be added to advertisedListeners config. This config should be treated as the advertised listeners for broker, not protocol handlers. What KafkaProtocolHandler#getProtocolDataToAdvertise returns is not used though it has been written to ZK. It also makes kafkaAdvertisedListeners meaningless and it was marked as deprecated.

Modification

Benefit by PIP 95, Pulsar client doesn't need to configure listener name for topic lookup. So this PR simplifies the implementation of LookupClient by using a shared PulsarClient without configuring listener name.

Then this PR uses the MetadataStoreCacheLoader introduced from #820 to get the kafkaAdvertisedListeners from ZK. Since it has already been cached, this PR removes the KOP_ADDRESS_CACHE.

To verify the feature, this PR adds a test testMetadataRequestForMultiListeners. The KoP config is like

kafkaListeners=PLAINTEXT://0.0.0.0:<port1>,GW://0.0.0.0:<port2>
kafkaProtocolMap=PLAINTEXT:PLAINTEXT,GW:PLAINTEXT
kafkaAdvertisedListeners=PLAINTEXT://192.168.0.1:<port3>,GW://192.168.0.1:<port4>

And verify that each KafkaRequestHandler can handle the METADATA requests well.

EronWright

All seems good to me.

kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/KopBrokerLookupManager.java

kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/KafkaServiceConfiguration.java

Demogorgon314

LGTM.

…teners (#864) Fixes #818 ### Motivation #742 adds the multiple listeners support for KoP 2.8.x. However, there're some flaws. First, the KoP advertised listeners must be added to `advertisedListeners` config. This config should be treated as the advertised listeners for broker, not protocol handlers. What `KafkaProtocolHandler#getProtocolDataToAdvertise` returns is not used though it has been written to ZK. It also makes `kafkaAdvertisedListeners` meaningless and it was marked as deprecated. ### Modification Benefit by [PIP 95](apache/pulsar#12040), Pulsar client doesn't need to configure listener name for topic lookup. So this PR simplifies the implementation of `LookupClient` by using a shared `PulsarClient` without configuring listener name. Then this PR uses the `MetadataStoreCacheLoader` introduced from #820 to get the `kafkaAdvertisedListeners` from ZK. Since it has already been cached, this PR removes the `KOP_ADDRESS_CACHE`. To verify the feature, this PR adds a test `testMetadataRequestForMultiListeners`. The KoP config is like ```properties kafkaListeners=PLAINTEXT://0.0.0.0:<port1>,GW://0.0.0.0:<port2> kafkaProtocolMap=PLAINTEXT:PLAINTEXT,GW:PLAINTEXT kafkaAdvertisedListeners=PLAINTEXT://192.168.0.1:<port3>,GW://192.168.0.1:<port4> ``` And verify that each `KafkaRequestHandler` can handle the METADATA requests well.

Cherry-picking #901 relies on some changes of #864, while #864 cannot be cherry-picked into branch-2.8.1 because it relies on PIP 95, which was introduced from Pulsar 2.9.0

…teners (streamnative#864) Fixes streamnative#818 ### Motivation streamnative#742 adds the multiple listeners support for KoP 2.8.x. However, there're some flaws. First, the KoP advertised listeners must be added to `advertisedListeners` config. This config should be treated as the advertised listeners for broker, not protocol handlers. What `KafkaProtocolHandler#getProtocolDataToAdvertise` returns is not used though it has been written to ZK. It also makes `kafkaAdvertisedListeners` meaningless and it was marked as deprecated. ### Modification Benefit by [PIP 95](apache/pulsar#12040), Pulsar client doesn't need to configure listener name for topic lookup. So this PR simplifies the implementation of `LookupClient` by using a shared `PulsarClient` without configuring listener name. Then this PR uses the `MetadataStoreCacheLoader` introduced from streamnative#820 to get the `kafkaAdvertisedListeners` from ZK. Since it has already been cached, this PR removes the `KOP_ADDRESS_CACHE`. To verify the feature, this PR adds a test `testMetadataRequestForMultiListeners`. The KoP config is like ```properties kafkaListeners=PLAINTEXT://0.0.0.0:<port1>,GW://0.0.0.0:<port2> kafkaProtocolMap=PLAINTEXT:PLAINTEXT,GW:PLAINTEXT kafkaAdvertisedListeners=PLAINTEXT://192.168.0.1:<port3>,GW://192.168.0.1:<port4> ``` And verify that each `KafkaRequestHandler` can handle the METADATA requests well. (cherry picked from commit 50699b3)

…brokers (#941) **Motivation** When you enable JWT token authentication transactions do not work because transactions need broker-to-broker communication but there is currently no support for the SASL handshake. **Changes** Now when you enable authentication and you configure Token Authentication for Broker-To-Broker communications in Pulsar, we perform SASL PLAIN authentication while preparing the connection to other brokers. **Tests** I have extended existing SASL tests in order to use Transactions. I have copy/pasted some parts of the TransactionTest, but there was not enough code to make sense to share the test code. Fix conflicts made by #864, which changes the signature of `lookupBroker`.

…with kafkaListeners (#1092) This PR is a follow up bug fix of #1042 for branch-2.8.2. It also migrate some util methods and tests from #864.

…espace (#1274) ### Motivation Currently in KoP tests, the metadata namespace configured by `kafkaMetadataNamespace` is "default", which is same with the default namespace of Pulsar and KoP (`kafkaNamespace`). https://github.com/streamnative/kop/blob/64001937c92d80ed4f652becd8d6b9768918b875/tests/src/test/java/io/streamnative/pulsar/handlers/kop/KopProtocolHandlerTestBase.java#L176-L177 It's a terrible config because some topic policies might be changed with the metadata namespace and the offset topic is created during starting. We should avoid the namespace for normal topics being affected by these implicit operations. ### Modifications Remove the configs for `kafkaMetadataTenant` and `kafkaMetadataNamespace`, then fix the following tests: - `MultiLedgerTest`: set the infinite retention to avoid rollover ledgers being removed automatically. - `KafkaTopicConsumerManagerTest`: trigger the topic lookup before creating `PersistentTopic` objects in `BrokerService` so that broker can acquire the ownership of namespace bundles. - `CacheInvalidatorTest`: unload the metadata namespace instead of the default namespace. (cherry picked from commit 9e402f8) Resolve the conflicts by printing the KOP_ADDRESS_CACHE, which was removed in branch-2.9.x and higher since #864.

Hongten · 2023-03-09T03:42:42Z

tests/src/test/java/io/streamnative/pulsar/handlers/kop/KafkaListenerNameTest.java

+    public void testMetadataRequestForMultiListeners() throws Exception {
+        final Map<Integer, InetSocketAddress> bindPortToAdvertisedAddress = new HashMap<>();
+        final int anotherKafkaPort = PortManager.nextFreePort();
+        bindPortToAdvertisedAddress.put(kafkaBrokerPort,


I encountered an error Listener 'PLAINTEXT://192.168.0.1/<unresolved>:15007' is invalid when calling super.internalSetup(). Did you have the same issue when you run this test?

BewareMyPower requested a review from jiazhai as a code owner October 28, 2021 07:42

BewareMyPower added 5 commits October 28, 2021 20:55

Correct topic lookup implementation

8eff8dd

Remove KOP_ADDRESS_CACHE

b65ea62

Store channel initializers for tests

a0b21c4

Add tests for findBroker

2e4828f

Test metadata request

0c8764f

BewareMyPower changed the title ~~[WIP] Correct topic lookup implementation~~ Support multiple listeners without configuring broker's advertisedListeners Oct 28, 2021

Remove unused protocol map

d0b09fe

BewareMyPower force-pushed the bewaremypower/multi-listener branch from 323cbbe to d0b09fe Compare October 28, 2021 13:33

BewareMyPower requested review from Demogorgon314 and EronWright October 28, 2021 13:33

BewareMyPower self-assigned this Oct 28, 2021

BewareMyPower added the type/feature Indicates new functionality label Oct 28, 2021

EronWright approved these changes Oct 29, 2021

View reviewed changes

BewareMyPower added doc-required This pr needs a document release/2.9.1 release/2.9.0 and removed release/2.9.1 labels Oct 29, 2021

Demogorgon314 reviewed Oct 29, 2021

View reviewed changes

kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/KopBrokerLookupManager.java Outdated Show resolved Hide resolved

Use orElseThrow instead of orElseGet

11950e1

Demogorgon314 reviewed Oct 29, 2021

View reviewed changes

kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/KopBrokerLookupManager.java Show resolved Hide resolved

EronWright reviewed Oct 29, 2021

View reviewed changes

kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/KafkaServiceConfiguration.java Outdated Show resolved Hide resolved

Fix config description

75c05bd

Demogorgon314 approved these changes Oct 29, 2021

View reviewed changes

BewareMyPower merged commit 50699b3 into streamnative:master Oct 29, 2021

BewareMyPower deleted the bewaremypower/multi-listener branch October 29, 2021 14:03

BewareMyPower mentioned this pull request Nov 9, 2021

[DOCS] How to enable multiple listeners for KoP 2.9.0 #889

Closed

BewareMyPower mentioned this pull request Nov 17, 2021

Make produce and fetch purgatory shared by all connections #901

Merged

BewareMyPower mentioned this pull request Feb 16, 2022

Fix CreateTopics failure when kafkaAdvertisedListeners are different with kafkaListeners #1092

Merged

BewareMyPower mentioned this pull request Mar 8, 2022

Confirm the behavior change for METADATA response since 2.9 and later #1142

Closed

BewareMyPower removed the doc-required This pr needs a document label Jan 10, 2023

Hongten reviewed Mar 9, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple listeners without configuring broker's advertisedListeners #864

Support multiple listeners without configuring broker's advertisedListeners #864

Uh oh!

BewareMyPower commented Oct 28, 2021 •

edited

Loading

Uh oh!

EronWright left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Demogorgon314 left a comment

Uh oh!

Hongten Mar 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Support multiple listeners without configuring broker's advertisedListeners #864

Support multiple listeners without configuring broker's advertisedListeners #864

Uh oh!

Conversation

BewareMyPower commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modification

Uh oh!

EronWright left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Demogorgon314 left a comment

Choose a reason for hiding this comment

Uh oh!

Hongten Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BewareMyPower commented Oct 28, 2021 •

edited

Loading