[PIP-105] Part-1 Support add subscription properties #12869

315157973 · 2021-11-18T09:00:12Z

Motivation

I divided this PIP into two parts, the first part supports setting properties for subscriptions
The second part will support plug-in loading, filtering, etc.

Modifications

Support add subscription properties

Documentation

[ x ] doc-required

codelipenghui · 2021-11-19T14:52:59Z

We have a short conversation for this PR, provide the context here.

We don't need to persist the subscription properties
Use a separate field for the subscription properties to distinguish the consumer metadata and subscription properties, otherwise, we need to maintain the subscription properties for each consumer.
The subscription properties follow the same way as the subscription type, the subscription applies the subscription properties of the first connected consumer.

Jason918

Why do we make the type of property value Long? Isn't String more extendable?

Jason918 · 2021-11-21T14:59:15Z

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


    optional KeySharedMeta keySharedMeta = 17;
+
+    repeated KeyLongValue subscription_properties = 18;


It should be optional ?

repeated already contains optional

Good to know, Thx~

It should be KeyValue? If use KeyLongValue, we can only use uint64 for the value.

# Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java

codelipenghui · 2021-11-22T06:44:15Z

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


    optional KeySharedMeta keySharedMeta = 17;
+
+    repeated KeyLongValue subscription_properties = 18;


It should be KeyValue? If use KeyLongValue, we can only use uint64 for the value.

pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SubscriptionOption.java

codelipenghui · 2021-11-22T07:05:02Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Topic.java

+     * @param option
+     * @return
+     */
+    CompletableFuture<Consumer> subscribe(SubscriptionOption option);


Looks like we can remove the old one to avoid interface complexity?

Deleting the interface is not good, I first set it to @deprecated

codelipenghui · 2021-11-22T07:17:38Z

...broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java

        this.fullName = MoreObjects.toStringHelper(this).add("topic", topicName).add("name", subName).toString();
        this.expiryMonitor = new PersistentMessageExpiryMonitor(topicName, subscriptionName, cursor, this);
        this.setReplicated(replicated);
+        this.subscriptionProperties = subscriptionProperties == null ? new HashMap<>() : subscriptionProperties;


Suggested change

this.subscriptionProperties = subscriptionProperties == null ? new HashMap<>() : subscriptionProperties;

this.subscriptionProperties = subscriptionProperties == null ? Collections.emptyMap() : Collections.unmodifiableMap(subscriptionProperties);

Collections.emptyMap() is a static Map, and data may be put into this Map, so I think it is better to new

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

…umerBuilderImpl.java Co-authored-by: lipenghui <penghui@apache.org>

…Commands.java Co-authored-by: lipenghui <penghui@apache.org>

wolfstudy · 2021-11-22T12:23:48Z

@315157973 In topics stats, Do we need to display subscription properties information?

When the user sets the sub properties, how do we view the sub properties set by the user?

hangc0276 · 2021-11-22T14:01:58Z

...-broker/src/main/java/org/apache/pulsar/broker/service/nonpersistent/NonPersistentTopic.java

+    }
+
    @Override
    public CompletableFuture<Consumer> subscribe(final TransportCnx cnx, String subscriptionName, long consumerId,


This interface has been marked as Deprecated, we'd better change it to private CompletableFuture<Consumer> internalSubscribe and delete @Override to avoid confusing.

hangc0276 · 2021-11-22T14:03:54Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Topic.java

    void recordAddLatency(long latency, TimeUnit unit);

+    @Deprecated
    CompletableFuture<Consumer> subscribe(TransportCnx cnx, String subscriptionName, long consumerId, SubType subType,


There are a lot of tests called this interface, and in order to delete it in the future, should we change the test to call the new one?

Okay, I will open another PR to do this

hangc0276 · 2021-11-22T14:22:13Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/CreateSubscriptionTest.java

+        assertEquals(properties4.get("3"), "3");
+        assertEquals(properties4.get("4"), "4");
+        consumer4.close();
+    }


add the case: consumer subscribe without subscriptionProperties set, it will get the old one. And then restart broker, it won't get any properties?

restart broker and create a new consumer with new properties, the properties will be updated. Do not restart broker and create a new consumer with new properties, the properties will be ignored. Whether the broker restart or not will not be known for consumer, does this action will confuse the consumer?

Unit test has been added

The configuration of the Consumer is usually the same. Even if the Broker restarts and the Consumer reconnects with the original configuration, the Properties will be restored.

315157973 · 2021-11-23T02:39:30Z

@315157973 In topics stats, Do we need to display subscription properties information?

When the user sets the sub properties, how do we view the sub properties set by the user?

There are still many things to do in the future, and different PRs will be split

hangc0276

Good job.

Signed-off-by: xiaolongran <xiaolongran@tencent.com> ### Motivation In #12869 , we introduce new filed `subscriptionProperties` in `CommandSubsctibe`. In order to better query the subscriptionProperties set by the user, we will display this property in SubscribeStats ### Modifications - Add subscription properties for SubscriptionStats - add test case

Signed-off-by: xiaolongran <xiaolongran@tencent.com> ### Motivation In apache/pulsar#12869, we introduce pluggable entry filter in Dispatcher, the pull request is the Go SDK implementation of this PIP ### Modifications *Describe the modifications you've done.* - Add subscription properties for ConsumerOptions - Update `pulsarApi.proto` file

### Motivation See apache#12269 I divided this PIP into two parts, the first part supports setting properties for subscriptions The second part will support plug-in loading, filtering, etc. ### Modifications Support add subscription properties

Signed-off-by: xiaolongran <xiaolongran@tencent.com> ### Motivation In apache#12869 , we introduce new filed `subscriptionProperties` in `CommandSubsctibe`. In order to better query the subscriptionProperties set by the user, we will display this property in SubscribeStats ### Modifications - Add subscription properties for SubscriptionStats - add test case

### Motivation See apache#12269 I divided this PIP into two parts, the first part supports setting properties for subscriptions The second part will support plug-in loading, filtering, etc. ### Modifications Support add subscription properties

Signed-off-by: xiaolongran <xiaolongran@tencent.com> ### Motivation In apache#12869 , we introduce new filed `subscriptionProperties` in `CommandSubsctibe`. In order to better query the subscriptionProperties set by the user, we will display this property in SubscribeStats ### Modifications - Add subscription properties for SubscriptionStats - add test case

Support add subscription properties

f83d8ad

315157973 requested review from BewareMyPower, codelipenghui, eolivelli, gaoran10 and hangc0276 November 18, 2021 09:00

315157973 self-assigned this Nov 18, 2021

github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Nov 18, 2021

315157973 added 2 commits November 18, 2021 19:25

fix checkstyle

3ec7541

fix checkstyle

ce0e442

Jason918 reviewed Nov 21, 2021

View reviewed changes

315157973 added 3 commits November 22, 2021 13:48

Properties are no longer persisted

8ab694c

Merge branch 'master' into sub-propertise

e916bd8

# Conflicts: # pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java

restore merge

dd8e3e0

codelipenghui reviewed Nov 22, 2021

View reviewed changes

codelipenghui added this to the 2.10.0 milestone Nov 22, 2021

315157973 and others added 5 commits November 22, 2021 15:25

Update pulsar-client/src/main/java/org/apache/pulsar/client/impl/Cons…

03ed28a

…umerBuilderImpl.java Co-authored-by: lipenghui <penghui@apache.org>

Update pulsar-common/src/main/java/org/apache/pulsar/common/protocol/…

54768b7

…Commands.java Co-authored-by: lipenghui <penghui@apache.org>

address comment

c8a6e0e

check style

781a64c

check style

172e1c2

codelipenghui approved these changes Nov 22, 2021

View reviewed changes

hangc0276 reviewed Nov 22, 2021

View reviewed changes

315157973 added 3 commits November 23, 2021 11:24

address comment

b97b94e

address comment

db01a29

check style

e2855eb

hangc0276 approved these changes Nov 23, 2021

View reviewed changes

hangc0276 added area/broker release/2.10.0 labels Nov 23, 2021

hangc0276 merged commit 6b8ebbd into apache:master Nov 24, 2021

wolfstudy mentioned this pull request Nov 25, 2021

Add subscription properties for SubscriptionStats #12979

Merged

wolfstudy mentioned this pull request Nov 29, 2021

Add subscription properties for ConsumerOptions apache/pulsar-client-go#671

Merged

Anonymitaet mentioned this pull request Jan 6, 2022

[Doc] add explanations for entry filter #13646

Merged

Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Jan 6, 2022


		optional KeySharedMeta keySharedMeta = 17;

		repeated KeyLongValue subscription_properties = 18;

	this.subscriptionProperties = subscriptionProperties == null ? new HashMap<>() : subscriptionProperties;
	this.subscriptionProperties = subscriptionProperties == null ? Collections.emptyMap() : Collections.unmodifiableMap(subscriptionProperties);

[PIP-105] Part-1 Support add subscription properties #12869

[PIP-105] Part-1 Support add subscription properties #12869

Uh oh!

Conversation

315157973 commented Nov 18, 2021

Motivation

Modifications

Documentation

Uh oh!

codelipenghui commented Nov 19, 2021

Uh oh!

Jason918 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!

Uh oh!

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!

315157973 Nov 22, 2021 • 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!

Uh oh!

wolfstudy commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

315157973 commented Nov 23, 2021

Uh oh!

hangc0276 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

315157973 Nov 22, 2021 •

edited

Loading

wolfstudy commented Nov 22, 2021 •

edited

Loading