-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[broker] Optimize TopicPolicies#maxUnackedMessagesOnConsumer with HierarchyTopicPolicies #13618
[broker] Optimize TopicPolicies#maxUnackedMessagesOnConsumer with HierarchyTopicPolicies #13618
Conversation
/pulsarbot run-failure-checks |
1 similar comment
/pulsarbot run-failure-checks |
19484c8
to
8c2681c
Compare
@codelipenghui @315157973 PTAL |
@@ -102,7 +102,7 @@ | |||
|
|||
private final ConsumerStatsImpl stats; | |||
|
|||
private volatile int maxUnackedMessages; | |||
private final boolean maxUnackedMessagesEnabled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we update Policies, it should be possible to disable this feature. If it is set to final, how to update it later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This maxUnackedMessagesEnabled
is used for the purpose of disabling "unacked messages check" feature on consumer level for non-durable subscriptions. Following previous logic, see
pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Lines 756 to 758 in af761e2
int maxUnackedMessages = isDurable | |
? getMaxUnackedMessagesOnConsumer() | |
: 0; |
e95c9d6
to
29ae27d
Compare
/pulsarbot run-failure-checks |
@Jason918 Please rebase to the master branch, there is a CPP test fix in the master branch |
43c4eba
to
914eef7
Compare
/pulsarbot run-failure-checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
…rarchyTopicPolicies (apache#13618)
Motivation
This is one of the serial topic policy optimization with
HierarchyTopicPolicies
.Update topic policy with HierarchyTopicPolicies comes with these benefits:
Modifications
maxUnackedMessagesOnConsumer
in org.apache.pulsar.broker.service.AbstractTopic#topicPolicies.maxUnackedMessages
tomaxUnackedMessagesEnabled
in Consumer.maxUnackedMessages
check is enabled only for durable cursor.Verifying this change
This change is already covered by existing tests, such as
Some checklist for updating topic policy with
HierarchyTopicPolicies
.Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
no-need-doc
Code optimization.