PIP-13-2/3: support regex based subscription #1279

zhaijack · 2018-02-24T03:47:52Z

Motivation

This is a second sub-task for pip-13, which would like to leverage the first task to support regex based subscription.

Modifications

add subscribe methods with regex pattern in PulsarClientImpl.
add PatternTopicsConsumerImpl, which extends TopicsConsumerImpl.
add a proto command to get topics under a namespace.
add a test to verify.

Result

old methods behaviour not changed,
user could use new method to subscribe to topics based on regex pattern

merlimat

Change looks good. Just left few comments

merlimat · 2018-02-24T21:20:35Z

pulsar-common/src/main/proto/PulsarApi.proto

@@ -136,7 +136,8 @@ enum ProtocolVersion {
 	v10 = 10;// Added proxy to broker
 	v11 = 11;// C++ consumers before this version are not correctly handling the checksum field
 	v12 = 12;// Added get topic's last messageId from broker
-                 // Added CommandActiveConsumerChange
+			 // Added CommandActiveConsumerChange
+			 // Added CommandGetTopicsOfNamespace


v12 got released in 1.22. We should bump the protocol version again

Thanks. will change it.

oh, checked again, seems 1.22 still use v11:
https://github.com/apache/incubator-pulsar/blob/branch-1.22/pulsar-common/src/main/proto/PulsarApi.proto#L137

merlimat · 2018-02-24T21:30:29Z

pulsar-common/src/main/proto/PulsarApi.proto

+
+message CommandGetTopicsOfNamespaceResponse {
+	required uint64 request_id	= 1;
+	repeated string topics 		= 2;


It's probably not worth it or even necessary to bother about, but I was thinking how we could compress a little be this response.

For one, we at least know that all topics have the same namespace (though they might have persistent and non-persistent topics together).

Thanks. How about transfer back only the last part of local_topics_name?
If this is really a issue, maybe we could do the filter in server side, and only transfer back matched topics? This may need add new command.

merlimat · 2018-02-24T21:33:41Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java

@@ -340,6 +344,53 @@ public Consumer subscribe(String topic, String subscription, ConsumerConfigurati
        return consumerSubscribedFuture;
    }

+    public CompletableFuture<Consumer> subscribeAsync(Pattern topicsPattern,


Take a look at the refactored code that is in master already. There is a single subscribeAsync(ConsumerConfigurationData conf) method that is used for all the variations of the subscribe.

Then, for this regex subscribe, we should reuse the same code that subscribe to the list of topics, just adding the extra pre-step of figuring out the list.

thanks, found it now, will reference it.

merlimat · 2018-02-24T21:36:40Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/HttpLookupService.java

+    public CompletableFuture<List<String>> getTopicsUnderNamespace(NamespaceName namespace) {
+        CompletableFuture<List<String>> future = new CompletableFuture<>();
+        httpClient
+            .get(String.format("admin/namespaces/%s/destinations", namespace), String[].class)


We should be able to deserialize directly into a List<String> with Jackson, by passing List.class.

Thanks, List<String> deserialise could be like this: List<String> names = mapper.readValue(jsonInput, new TypeReference<List<String>>(){}); while httpclient.get() not support this way of TypeReference currently. So make a String[] to leverage current code.

merlimat · 2018-02-24T21:40:11Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ConsumerBuilder.java

+     *
+     * @param topicsPattern
+     */
+    ConsumerBuilder topicsPattern(Pattern topicsPattern);


I would prefer to just take String here for the pattern, since most of the time the regex will be very simple.

We can also offer both versions:

ConsumerBuilder topicsPattern(Pattern topicsPattern); ConsumerBuilder topicsPattern(String topicsPattern);

Thanks, will add it.

@zhaijack Actually, I was thinking that probably, using a "glob" pattern should be enough in most cases.

I would expect that almost all usages of this API would be like
"topic-category-*" or very similar.

I cannot really see a case in which a full blown regex is indeed needed.

What do you think?

:) @merlimat , yes, seems most of the case is like that you mentioned. Not sure if user's use case would be different as we thought. It maybe better to provide a wider compatibility for api. Seems Kafka also use this java.util.regex.Pattern.
How about leave it as you suggested above?:

ConsumerBuilder topicsPattern(Pattern topicsPattern); ConsumerBuilder topicsPattern(String topicsPattern);

+1 for having both String and Pattern builders.

merlimat · 2018-02-24T21:43:07Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/BinaryProtoLookupService.java

+                                                                    NamespaceName namespace) {
+        CompletableFuture<List<String>> topicsFuture = new CompletableFuture<List<String>>();
+
+        client.getCnxPool().getConnection(socketAddress).thenAccept(clientCnx -> {


At the higher level, we should have a way to retry few times this call, otherwise if we're unlike when starting our consumer we might get exception even if there is a brief transient problem (eg: 1 single broker going down).

Thanks, will add it.

…n method

jiazhai · 2018-02-26T06:26:17Z

updated. Thanks @merlimat

sijie · 2018-02-26T09:03:01Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ConsumerBuilder.java

+     *
+     * @param topicsPattern
+     */
+    ConsumerBuilder topicsPattern(Pattern topicsPattern);


+1 for having both String and Pattern builders.

merlimat

👍 I just have one tiny comment on expanding a little bit the Javadoc for the new methods in the builder.

merlimat · 2018-02-27T00:54:27Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/ConsumerBuilder.java

+    ConsumerBuilder topicsPattern(Pattern topicsPattern);
+
+    /**
+     * Specify a pattern for topics that this consumer will subscribe on.


Can you explain a bit more in detail what kind of pattern is accepted (say regex/glob .. ) and also provide a short example snippet on how to use this.

Thanks, will add it.

…ribe (#2219) In PR #1279 and #1298 we added regex based subscription. This is a catch up work to add `PatternMultiTopicsConsumerImpl` in cpp client.

zhaijack force-pushed the pip-13-2 branch from 81ba09d to 7b7964e Compare February 24, 2018 04:01

merlimat assigned zhaijack Feb 24, 2018

merlimat added this to the 2.0.0-incubating milestone Feb 24, 2018

merlimat added the type/feature The PR added a new feature or issue requested a new feature label Feb 24, 2018

merlimat reviewed Feb 24, 2018

View reviewed changes

zhaijack force-pushed the pip-13-2 branch from e237d04 to c947516 Compare February 26, 2018 05:37

jiazhai added 3 commits February 26, 2018 13:50

leverage lookup service to getTopicsOfNamespace, add subscribe patter…

6ec5232

…n method

change following @Matteo's comments

ea6c60d

fix rebase error

2765ae8

zhaijack force-pushed the pip-13-2 branch from c947516 to 2765ae8 Compare February 26, 2018 06:17

sijie approved these changes Feb 26, 2018

View reviewed changes

merlimat approved these changes Feb 27, 2018

View reviewed changes

add comments for pattern in builder

da1e0ae

zhaijack force-pushed the pip-13-2 branch from 51fabbe to da1e0ae Compare February 27, 2018 03:26

merlimat merged commit ba58a2a into apache:master Feb 27, 2018

This was referenced Feb 28, 2018

PIP-13-2: support regex based subscription #1101

Closed

Documentation for regex_pattern_subscribe #1176

Closed

zhaijack mentioned this pull request Jul 24, 2018

Cpp client: add PatternMultiTopicsConsumerImpl to support regex subscribe #2219

Merged

jiazhai mentioned this pull request Mar 4, 2019

support Pattern subscribe in kafka compat client #3733

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PIP-13-2/3: support regex based subscription #1279

PIP-13-2/3: support regex based subscription #1279

zhaijack commented Feb 24, 2018

merlimat left a comment

merlimat Feb 24, 2018

zhaijack Feb 25, 2018

zhaijack Feb 25, 2018

merlimat Feb 24, 2018

zhaijack Feb 26, 2018 •

edited

merlimat Feb 24, 2018

zhaijack Feb 25, 2018 •

edited

merlimat Feb 24, 2018

zhaijack Feb 25, 2018 •

edited

merlimat Feb 24, 2018

zhaijack Feb 25, 2018

merlimat Feb 26, 2018

zhaijack Feb 26, 2018 •

edited

sijie Feb 26, 2018

merlimat Feb 24, 2018

zhaijack Feb 25, 2018

jiazhai commented Feb 26, 2018

sijie Feb 26, 2018

merlimat left a comment

merlimat Feb 27, 2018

zhaijack Feb 27, 2018

PIP-13-2/3: support regex based subscription #1279

PIP-13-2/3: support regex based subscription #1279

Conversation

zhaijack commented Feb 24, 2018

Motivation

Modifications

Result

merlimat 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

zhaijack Feb 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaijack Feb 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaijack Feb 25, 2018 • edited

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

zhaijack Feb 26, 2018 • edited

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

jiazhai commented Feb 26, 2018

Choose a reason for hiding this comment

merlimat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaijack Feb 26, 2018 •

edited

zhaijack Feb 25, 2018 •

edited

zhaijack Feb 25, 2018 •

edited

zhaijack Feb 26, 2018 •

edited