Allow non-pesistent topics to be retrieved along with persistent ones from the "GetTopicsOfNamespace" method #2025

gordeevbr · 2018-06-24T21:42:34Z

Motivation

Please see issue #2009 for a detailed bug report.

In our use case we require using java client with pattern subscription to read from a set of non-persistent topics. Unfortunately, right now this feature doesn't work. After researching the cause for this I have found out that under the hood the client is requesting a list of topics by namespace from the server and then filters them out by pattern and subscribes to them. The method in Pulsar broker NamespaceService class that is responsible for searching for required topics only uses ledgers, thus returning only persistent topics to the client. The goal of this pull request is to provide a solution for that problem.

Modifications

This pull request updates getListOfTopics method of NamespaceService class to also include active non-pesistent topics from local broker cache inside the multiLayerTopicsMap collection of BrokerService in the result.

Result

As a result, requesting a list of topics by namespace using the HTTP API or binary API (and thus via the clients) will add non-persistent topics to search result, allowing pattern subscription to be used with non-persistent topics.

Considerations

Since this method pulls non-persistent topics from local broker cache, this probably means that this solution will only work for Pulsar installations with a single broker. And if there are multiple brokers, results might be inconsistent. Unfortunately I don't really know if non-persistent themselves work in multi-broker setups. I have recently asked on Slack if non-persistent topics are being replicated in any way and @merlimat's response was that they don't. Also it seems to be that some other methods that are working with non-persistent topics are using this very same collection.
It seems to me that unit tests have made sure that Java client can work with this setup, but this might still be a breaking change for other clients or if applications working with this API are not expecting non-persistent topics in result.
I have made sure that old unit tests inside the pulsar-broker subproject are still working and updated some old tests for this particular use case. Are there any more tests that I can add.

Overall, we really need this and I would appreciate if maintainers could share their opinion. Thanks in advance.

sijie · 2018-06-25T05:09:15Z

@jiazhai please review this when you have time.

gordeevbr · 2018-06-25T07:34:53Z

Just noticed integration tests are failing. Is this my fault? Should I fix that?

srkukarni · 2018-06-25T07:59:57Z

retest this please

merlimat · 2018-06-25T17:41:54Z

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

        }

+        // Non-persistent topics don't have managed ledges so we have to retrieve them from local cache.
+		synchronized (pulsar.getBrokerService().getMultiLayerTopicMap()) {


This will only answer with the non-persistent topics served by the broker that it's handling the particular request. However, topics for a single namespace can be spread across multiple brokers.

We would need to do something like this ( https://github.com/apache/incubator-pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/NonPersistentTopics.java#L170 ) to get the correct list of non-persistent topics.

I think the best option is to actually use the same API to get the list of non-persistent topics through HTTP. GET /admin/v2/non-persistent/{tenant}/{namespace}. That should take care of everything.

merlimat · 2018-06-25T18:00:57Z

Unfortunately I don't really know if non-persistent themselves work in multi-broker setups. I have recently asked on Slack if non-persistent topics are being replicated in any way and @merlimat's response was that they don't. Also it seems to be that some other methods that are working with non-persistent topics are using this very same collection.

A single topic (or partition of a topic) is assigned to a single broker, at a given point in time.
Topics belonging to a namespace can be assigned to different brokers (that is true for both persistent and non-persistent topics)
Persistent topics use BookKeeper to persist data and maintain state --> When a broker crashes, the topic can move immediately to a different broker and restart with the same exact state.
Non-persistent topics do not store any kind of state durably. The messages are kept for a very short amount of time in memory in the broker and dropped if they cannot be delivered immediately. The data for a single non-persistent topic is not stored in multiple machines. If the broker crashes, some messages will be lost and the topic will continue on the new broker.

jiazhai · 2018-06-25T23:28:33Z

pulsar-broker/src/test/java/org/apache/pulsar/client/impl/PatternTopicsConsumerImplTest.java

        final String topicName2 = "persistent://my-property/my-ns/topic-2-" + key;
        final String topicName3 = "persistent://my-property/my-ns/topic-3-" + key;
-        List<String> topicNames = Lists.newArrayList(topicName1, topicName2, topicName3);
+		final String topicName4 = "non-persistent://my-property/my-ns/topic-4-" + key;


seems some space alignment issue.

jiazhai · 2018-06-25T23:28:55Z

pulsar-broker/src/test/java/org/apache/pulsar/client/impl/PatternTopicsConsumerImplTest.java

        String topicName1 = "persistent://my-property/my-ns/pattern-topic-1-" + key;
        String topicName2 = "persistent://my-property/my-ns/pattern-topic-2-" + key;
        String topicName3 = "persistent://my-property/my-ns/pattern-topic-3-" + key;
+		final String topicName4 = "non-persistent://my-property/my-ns/pattern-topic-4-" + key;


jiazhai

Thanks for the fix. Test case looks good. As @merlimat point out, there is better way to get the list.

gordeevbr · 2018-06-26T06:15:34Z

Thanks for reviewing this everyone.
@merlimat we expected this feature to work through Pulsar Java client. I guess to use that API we will have to write our own client logic that queries for non-persistent topics first. I will take a look.
@jiazhai I must have forgotten my IDE is configured to use 2 whitespaces.

merlimat · 2018-06-26T14:06:57Z

@merlimat we expected this feature to work through Pulsar Java client. I guess to use that API we will have to write our own client logic that queries for non-persistent topics first. I will take a look.

Yes, I was meaning that in broker side we should use the client API (there are already several cases that already do that). Since a single broker will not have all the topics, and that non-persistent topics don't have additional metadata, the client API call is the only option to get the correct list of non-persistent topics.

An additional improvement could be to only make this call if we know the client is interested in non-persistent topics and skip it otherwise.

…request.

…ests.

…tent topics.

gordeevbr · 2018-07-07T23:53:36Z

@merlimat @jiazhai I have updated this feature as you suggested.
I haven't yet added tests for this or fixed any old ones, just made sure it compiles. I will test this ASAP, just wanted to make sure this functionality is OK before I go any further.

This is what was done in the latest commits:

Additional property "Mode" was added to "CommandGetTopicsOfNamespace" entity of Pulsar binary API via Protobuf. This is a enum that has 3 values: "Persistent", "Non-Persistent", or "All". Value "Persistent" was set to be the default one.
Broker binary protocol server code was updated to retrieve topics from lookup based on this enum. More importantly, broker will attempt to retrieve a list of non-persistent topics from another cluster if necessary. A pool of Pulsar Clients was made for that purpose inside NamespaceService.
I have attempted to do the same for HTTP APIs, but I have only done so for API V1. I am not sure what to do about API V2, as it seems to use a somewhat more complicated functionality.
Client code was updated to support this. When building a consumer it is now possible to specify to which kind of topics you want to subscribe when using pattern subscription.

Will be glad to receive your feedback.

sijie · 2018-07-09T20:11:49Z

@merlimat @jiazhai can you review this PR and give @gordeevbr some feedback?

sijie · 2018-08-02T21:39:24Z

ping @merlimat and @jiazhai . lets give some feedback to @gordeevbr , so that we can move forward with the change here.

jiazhai · 2018-08-06T00:57:12Z

retest this please

jiazhai · 2018-08-06T01:07:09Z

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

-import org.apache.pulsar.common.policies.data.AuthAction;
-import org.apache.pulsar.common.policies.data.BacklogQuota;
+import org.apache.pulsar.common.api.proto.PulsarApi.CommandGetTopicsOfNamespace.Mode;
+import org.apache.pulsar.common.policies.data.*;


we should avoid using * in import.

jiazhai · 2018-08-06T01:17:45Z

pulsar-broker/src/test/java/org/apache/pulsar/client/impl/PatternTopicsConsumerImplTest.java

+        List<String> topics = ((PatternMultiTopicsConsumerImpl<?>) consumer).getPartitionedTopics();
+        List<ConsumerImpl<byte[]>> consumers = ((PatternMultiTopicsConsumerImpl<byte[]>) consumer).getConsumers();
+
+        assertEquals(topics.size(), 1);


Test failed at here.
Before patternconsumer created, the producer had been created. By design, it should have 7 topics (1+2+3+1)?

jiazhai

@gordeevbr The mode change looks good to me, Please go ahead.

FYI. Seems this caller also need fix:

2018-08-05\T\16:42:17.037 [ERROR] /home/jenkins/jenkins-slave/workspace/pulsar_precommit_cpp/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/LookupProxyHandler.java:[335,31] method newGetTopicsOfNamespaceRequest in class org.apache.pulsar.common.api.Commands cannot be applied to given types;
2018-08-05\T\16:42:17.037 [ERROR]   required: java.lang.String,long,org.apache.pulsar.common.api.proto.PulsarApi.CommandGetTopicsOfNamespace.Mode
2018-08-05\T\16:42:17.037 [ERROR]   found: java.lang.String,long
2018-08-05\T\16:42:17.037 [ERROR]   reason: actual and formal argument lists differ in length

gordeevbr · 2018-08-06T08:12:58Z

Thanks for your feedback.
Yes, I believe I have messed up some tests with this. I shall fix this as soon as possible.

…tyle.

gordeevbr · 2018-09-04T21:42:28Z

@jiazhai @merlimat @sijie

I have fixed old tests that were failing, added some new tests to test suites where it was possible, updated features to be consistent with HTTP Lookup, fixed all found issues, and updated with master branch.

There's not much new in these commits, mostly fixes and tests.

I think it should be ready now. I am, of course, ready to update this PR if deemed necessary.

sijie · 2018-09-04T21:44:26Z

@gordeevbr cool, thanks. I removed "[wip]". so other people know this PR is ready to review.

jiazhai

+1, Thanks for the work.

…ersistent

gordeevbr · 2018-09-12T13:46:31Z

Would you mind reruning these tests? I'm quite sure some of these were working just now.

…ersistent

… into GetListOfTopicsNonPersistent

sijie · 2018-09-23T23:40:12Z

run java8 tests

sijie · 2018-09-24T07:48:43Z

retest this please

sijie · 2018-09-24T23:33:52Z

run java8 tests

gordeevbr · 2018-09-25T06:11:05Z

Great. Thank you very much!

) ### Motivation Since #2025 has added query param - `Mode`, but the CLI is still not supported. #15410 has introduced to filter system topic. So this patch is supported the above query params for Namespace#getTopics.

VijayRohra · 2024-03-22T14:28:24Z

This is not working for me in Pulsar 2.11.1. I am able to receive messages from non-persistent topics belonging to single broker only.

Allowed non-pesistent topics to be retrieved along with persistent ones.

2531941

gordeevbr changed the title ~~Allowed non-pesistent topics to be retrieved along with persistent ones.~~ Allow non-pesistent topics to be retrieved along with persistent ones Jun 24, 2018

gordeevbr changed the title ~~Allow non-pesistent topics to be retrieved along with persistent ones~~ Allow non-pesistent topics to be retrieved along with persistent ones from the "GetTopicsOfNamespace" method Jun 24, 2018

sijie requested a review from jiazhai June 25, 2018 05:08

sijie assigned gordeevbr Jun 25, 2018

sijie added the area/client label Jun 25, 2018

sijie added this to the 2.2.0-incubating milestone Jun 25, 2018

merlimat reviewed Jun 25, 2018

View reviewed changes

jiazhai reviewed Jun 25, 2018

View reviewed changes

gordeevbr added 2 commits June 28, 2018 23:09

Updated Protobuf API to include optional "mode" field for namespaces …

6a2a446

…request.

Separated topics retrieval into two methods and fixed formatting in t…

8ac2dbf

…ests.

gordeevbr changed the title ~~Allow non-pesistent topics to be retrieved along with persistent ones from the "GetTopicsOfNamespace" method~~ [WiP] Allow non-pesistent topics to be retrieved along with persistent ones from the "GetTopicsOfNamespace" method Jun 30, 2018

Implemented additional functionality to support requesting non-persis…

0964211

…tent topics.

Merge branch 'master' into GetListOfTopicsNonPersistent

ce0cc1d

jiazhai reviewed Aug 6, 2018

View reviewed changes

gordeevbr added 4 commits August 8, 2018 23:16

Changed code style concerning imports to adhere to recommended code s…

17cbbec

…tyle.

Fixed old broken tests. Fixed an issue with client impl.

a4cdf4b

Enabled http pattern lookup with v2 API.

b406f6f

Merge branch 'master' into GetListOfTopicsNonPersistent

7017b6b

sijie changed the title ~~[WiP] Allow non-pesistent topics to be retrieved along with persistent ones from the "GetTopicsOfNamespace" method~~ Allow non-pesistent topics to be retrieved along with persistent ones from the "GetTopicsOfNamespace" method Sep 4, 2018

jiazhai approved these changes Sep 12, 2018

View reviewed changes

Merge remote-tracking branch 'apache/master' into GetListOfTopicsNonP…

c0eec88

…ersistent

Merge branch 'master' into GetListOfTopicsNonPersistent

e73b106

sijie mentioned this pull request Sep 19, 2018

Unable to get messages from non-persistent topics #2009

Closed

sijie added 2 commits September 20, 2018 00:55

Merge remote-tracking branch 'apache/master' into GetListOfTopicsNonP…

a689a4d

…ersistent

Merge remote-tracking branch 'gordeevbr/GetListOfTopicsNonPersistent'…

c3dab4f

… into GetListOfTopicsNonPersistent

sijie merged commit 6907135 into apache:master Sep 25, 2018

gordeevbr deleted the GetListOfTopicsNonPersistent branch September 25, 2018 06:11

itaross mentioned this pull request Apr 29, 2020

Subscription to topics with regex not working for non-persisten topics #6845

Open

sijie mentioned this pull request Apr 29, 2020

ISSUE-6845: Subscription to topics with regex not working for non-persisten topics streamnative/pulsar-archived#930

Open

Technoboy- mentioned this pull request Jun 22, 2022

[feature][CLI] Add query options to the get topics in Namespace. #16167

Merged

1 task

eolivelli mentioned this pull request Sep 12, 2022

[Bug] Delete Tenant Forcefully is broken if issued to a broker that is not owning all the topics/namespaces #17588

Closed

2 tasks

shibd mentioned this pull request Mar 13, 2023

PIP-258: Deprecation of the consumer subscriptionTopicsMode configuration #19798

Closed

sijie mentioned this pull request Mar 13, 2023

ISSUE-19798: PIP-258: Deprecation of the consumer subscribeTopicMode configuration streamnative/pulsar-archived#5618

Open

shibd mentioned this pull request Mar 27, 2023

[feat] Support pattern subscribe non-persistent topic. apache/pulsar-client-cpp#207

Merged

4 tasks

visortelle mentioned this pull request Apr 19, 2024

Topic creation not working properly #22527

Closed

3 tasks

Allow non-pesistent topics to be retrieved along with persistent ones from the "GetTopicsOfNamespace" method #2025

Allow non-pesistent topics to be retrieved along with persistent ones from the "GetTopicsOfNamespace" method #2025

Uh oh!

Conversation

gordeevbr commented Jun 24, 2018

Motivation

Modifications

Result

Considerations

Uh oh!

sijie commented Jun 25, 2018

Uh oh!

gordeevbr commented Jun 25, 2018

Uh oh!

srkukarni commented Jun 25, 2018

Uh oh!

merlimat Jun 25, 2018 • 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!

merlimat commented Jun 25, 2018

Uh oh!

jiazhai Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

jiazhai Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

jiazhai left a comment

Choose a reason for hiding this comment

Uh oh!

gordeevbr commented Jun 26, 2018

Uh oh!

merlimat commented Jun 26, 2018

Uh oh!

gordeevbr commented Jul 7, 2018

Uh oh!

sijie commented Jul 9, 2018

Uh oh!

sijie commented Aug 2, 2018

Uh oh!

jiazhai commented Aug 6, 2018

Uh oh!

jiazhai Aug 6, 2018

Choose a reason for hiding this comment

Uh oh!

jiazhai Aug 6, 2018

Choose a reason for hiding this comment

Uh oh!

jiazhai left a comment • 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!

gordeevbr commented Aug 6, 2018

Uh oh!

gordeevbr commented Sep 4, 2018

Uh oh!

sijie commented Sep 4, 2018

Uh oh!

jiazhai left a comment

Choose a reason for hiding this comment

Uh oh!

gordeevbr commented Sep 12, 2018

Uh oh!

sijie commented Sep 23, 2018

Uh oh!

sijie commented Sep 24, 2018

Uh oh!

sijie commented Sep 24, 2018

Uh oh!

gordeevbr commented Sep 25, 2018

Uh oh!

VijayRohra commented Mar 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

merlimat Jun 25, 2018 •

edited

Loading

jiazhai left a comment •

edited

Loading