[broker]Optimize topicMaxMessageSize with topic local cache. #12830

Jason918 · 2021-11-16T08:54:04Z

Motivation

Currently, , we update the value of Topic#topicMaxMessageSize in an interval of maxMessageSizeCheckIntervalInSeconds (default as 60s).
So if we update this value, we will have a delay, max to 60s by default, before it take effects.
We can reduce the latency by updating the value with notification.

Further, we can avoid the blank updates in isExceedMaximumMessageSize.

Modifications

Move topicMaxMessageSize to HierarchyTopicPolicies , and update by onUpdate listener, like other topic policies.
Deprecates maxMessageSizeCheckIntervalInSeconds, since it has no use any more.
Remove lastTopicMaxMessageSizeCheckTimeStamp and topicMaxMessageSizeCheckIntervalMs, no use any more.
Move method registerTopicPolicyListener up to AbstractTopic level, since topicMaxMessageSize also effect non-persist topics.

Verifying this change

Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

(example:)

org.apache.pulsar.broker.admin.TopicPoliciesTest#testTopicMaxMessageSize, and added test case for non-persisitent topics.
Add org.apache.pulsar.broker.admin.TopicPoliciesTest#testTopicPolicyInitValue to cover the case that topic loading old settings.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

doc

Doc updated in comments.
And checked with old configuration doc, no maxMessageSizeCheckIntervalInSeconds found, and no need to add since it is deprecated.

Jason918 · 2021-11-16T13:09:20Z

/pulsarbot run-failure-checks

Jason918 · 2021-11-17T02:10:27Z

/pulsarbot run-failure-checks

Jason918 · 2021-11-17T08:50:36Z

/pulsarbot run-failure-checks

Jason918 · 2021-11-20T02:14:05Z

@codelipenghui @315157973 Please help take a look.

codelipenghui

I think you missed the updateTopicPolicyByBrokerConfig to apply the default broker configuration?

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

Jason918 · 2021-11-22T09:31:20Z

I think you missed the updateTopicPolicyByBrokerConfig to apply the default broker configuration?

Not really. You mean ServiceConfiguration#maxMessageSize, it take effects in the netty RPC layer.
See

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

Line 130 in 3a337b8

    
           brokerConf.getMaxMessageSize() + Commands.MESSAGE_SIZE_FRAME_PADDING, 0, 4, 0, 4));

Jason918 · 2021-11-22T09:32:29Z

I think you missed the updateTopicPolicyByBrokerConfig to apply the default broker configuration?

Not really. You mean ServiceConfiguration#maxMessageSize, it take effects in the netty RPC layer.
See

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

Line 130 in 3a337b8

    
           brokerConf.getMaxMessageSize() + Commands.MESSAGE_SIZE_FRAME_PADDING, 0, 4, 0, 4));

codelipenghui · 2021-11-22T14:06:14Z

Yes, but I think we should keep consistent in the local topic policy cache? And for the next step, we can apply the PolicyHierarchyValue to get topic policy.

Jason918 · 2021-11-22T15:18:35Z

Yes, but I think we should keep consistent in the local topic policy cache? And for the next step, we can apply the PolicyHierarchyValue to get topic policy.

Great point. Added. And it's easier to find out which policy have broker/ns/topic level setting.

Jason918 · 2021-11-23T01:47:58Z

/pulsarbot run-failure-checks

codelipenghui

Could you please show me where update the PolicyHierarchyValue.brokerValue?

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

Jason918 · 2021-11-26T03:46:38Z

Could you please show me where update the PolicyHierarchyValue.brokerValue?

Last line in method updateTopicPolicyByBrokerConfig

delete ServiceConfiguration#maxMessageSizeCheckIntervalInSeconds set broker level value for TopicMaxMessageSize fix test failuer Fix tests fix test use DataProvider in testTopicMaxMessageSize

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

pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/PolicyHierarchyValue.java

* up/master: (75 commits) [website][upgrade]feat: website upgrade / docs migration - 2.5.1 Get Started/Concepts and Architecture/Pulsar Schema (apache#13030) Fix environment variable assignment in startup scripts (apache#13025) update 2.8.x (apache#13029) [Doc] add tips for Pulsar tools (apache#13044) Suggest to use tlsPort instead of deprecated TlsEnable (apache#13039) Integration tests for function-worker rebalance and drain operations. (apache#13058) fix(functions): missing runtime set in GoInstanceConfig (apache#13031) [pulsar-admin] Add get-replicated-subscription-status command for topic (apache#12891) [Broker] Consider topics in pulsar/system namespace as system topics (apache#13050) Fix typo: correct sizeUint to sizeUnit (apache#13040) fix-12894 (apache#12896) Don't attempt to delete pending ack store unless transactions are enabled (apache#13041) [Perf] Evaluate the current protocol version once (apache#13045) Fix Issue apache#12885, Unordered consuming case in Key_Shared subscription (apache#12890) [broker]Optimize topicMaxMessageSize with topic local cache. (apache#12830) [PIP-105] Part-2 Support pluggable entry filter in Dispatcher (apache#12970) [website] Modify admin-api-topic.md document (apache#12996) add missed import (apache#13037) [metadata] Add RocksdbMetadataStore (apache#12776) [C] Add pulsar_client_subscribe_multi_topics and pulsar_client_subscribe_pattern (apache#12965) ... # Conflicts: # site2/website-next/docusaurus.config.js # site2/website-next/versioned_sidebars/version-2.6.1-sidebars.json # site2/website-next/versions.json

…12830) ### Motivation Currently, , we update the value of `Topic#topicMaxMessageSize` in an interval of `maxMessageSizeCheckIntervalInSeconds` (default as 60s). So if we update this value, we will have a delay, max to 60s by default, before it take effects. We can reduce the latency by updating the value with notification. Further, we can avoid the blank updates in `isExceedMaximumMessageSize`. ### Modifications 1. Move `topicMaxMessageSize` to HierarchyTopicPolicies , and update by `onUpdate` listener, like other topic policies. 2. Deprecates `maxMessageSizeCheckIntervalInSeconds`, since it has no use any more. 3. Remove `lastTopicMaxMessageSizeCheckTimeStamp` and `topicMaxMessageSizeCheckIntervalMs`, no use any more. 4. Move method `registerTopicPolicyListener` up to AbstractTopic level, since `topicMaxMessageSize` also effect non-persist topics.

github-actions bot assigned Jason918 Nov 16, 2021

github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Nov 16, 2021

Jason918 force-pushed the opt_tp_topicMaxMessageSize branch from 9398202 to 1ed6d3d Compare November 22, 2021 05:16

codelipenghui closed this Nov 22, 2021

codelipenghui reopened this Nov 22, 2021

codelipenghui requested changes Nov 22, 2021

View reviewed changes

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java Outdated Show resolved Hide resolved

codelipenghui added this to the 2.10.0 milestone Nov 22, 2021

codelipenghui added the release/2.9.1 label Nov 22, 2021

codelipenghui removed the release/2.9.1 label Nov 22, 2021

Jason918 requested a review from codelipenghui November 23, 2021 02:03

codelipenghui requested review from 315157973 and hangc0276 November 25, 2021 14:04

codelipenghui reviewed Nov 25, 2021

View reviewed changes

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicPoliciesTest.java Outdated Show resolved Hide resolved

Jason918 force-pushed the opt_tp_topicMaxMessageSize branch from ebe97ee to 5f71429 Compare November 26, 2021 03:44

Jason918 requested a review from codelipenghui November 26, 2021 10:05

codelipenghui approved these changes Nov 26, 2021

View reviewed changes

Jason918 mentioned this pull request Nov 27, 2021

[broker] Optimize TopicPolicy#deduplicationEnabled with HierarchyTopicPolicies #13000

Merged

6 tasks

Optimize topicMaxMessageSize with topic local policy cache.

df88470

delete ServiceConfiguration#maxMessageSizeCheckIntervalInSeconds set broker level value for TopicMaxMessageSize fix test failuer Fix tests fix test use DataProvider in testTopicMaxMessageSize

Jason918 force-pushed the opt_tp_topicMaxMessageSize branch from 5f71429 to df88470 Compare November 30, 2021 03:51

315157973 reviewed Nov 30, 2021

View reviewed changes

pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicPoliciesTest.java Show resolved Hide resolved

pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/PolicyHierarchyValue.java Outdated Show resolved Hide resolved

update by comment and fix compile issue.

a521408

Jason918 requested a review from 315157973 November 30, 2021 05:56

315157973 approved these changes Nov 30, 2021

View reviewed changes

codelipenghui merged commit 1b20a97 into apache:master Nov 30, 2021

Jason918 mentioned this pull request Jan 14, 2022

[broker] Optimize topic policy with HierarchyTopicPolicies #13756

Closed

23 tasks

sijie mentioned this pull request Jan 14, 2022

ISSUE-13756: [broker] Optimize topic policy with HierarchyTopicPolicies streamnative/pulsar-archived#3567

Closed

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[broker]Optimize topicMaxMessageSize with topic local cache. #12830

[broker]Optimize topicMaxMessageSize with topic local cache. #12830

Jason918 commented Nov 16, 2021 •

edited

Loading

Jason918 commented Nov 16, 2021

Jason918 commented Nov 17, 2021

Jason918 commented Nov 17, 2021

Jason918 commented Nov 20, 2021

codelipenghui left a comment

Jason918 commented Nov 22, 2021

Jason918 commented Nov 22, 2021

codelipenghui commented Nov 22, 2021

Jason918 commented Nov 22, 2021

Jason918 commented Nov 23, 2021

codelipenghui left a comment

Jason918 commented Nov 26, 2021

[broker]Optimize topicMaxMessageSize with topic local cache. #12830

[broker]Optimize topicMaxMessageSize with topic local cache. #12830

Conversation

Jason918 commented Nov 16, 2021 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Jason918 commented Nov 16, 2021

Jason918 commented Nov 17, 2021

Jason918 commented Nov 17, 2021

Jason918 commented Nov 20, 2021

codelipenghui left a comment

Choose a reason for hiding this comment

Jason918 commented Nov 22, 2021

Jason918 commented Nov 22, 2021

codelipenghui commented Nov 22, 2021

Jason918 commented Nov 22, 2021

Jason918 commented Nov 23, 2021

codelipenghui left a comment

Choose a reason for hiding this comment

Jason918 commented Nov 26, 2021

Jason918 commented Nov 16, 2021 •

edited

Loading