Skip to content
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

Support set MaxUnackMessagesPerSubscription on topic level #7802

Merged
merged 4 commits into from
Aug 12, 2020

Conversation

315157973
Copy link
Contributor

Motivation
support set MaxUnackMessagesPerSubscription on topic level

Modifications
Support set/get/remove MaxUnackMessagesPerSubscription policy on topic level.

Verifying this change
Added Unit test to verify set/get/remove MaxUnackMessagesPerSubscription policy at Topic level work as expected when Topic level policy is enabled/disabled

org.apache.pulsar.broker.admin.MaxUnackedMessagesTest#testMaxUnackedMessagesOnSubscriptionApi
org.apache.pulsar.broker.admin.MaxUnackedMessagesTest#testMaxUnackedMessagesOnSubscription

@codelipenghui codelipenghui self-assigned this Aug 12, 2020
@codelipenghui codelipenghui added this to the 2.7.0 milestone Aug 12, 2020
@codelipenghui
Copy link
Contributor

@MarvinCai @jianyun8023 Could you please help review this PR?

@jianyun8023
Copy link
Contributor

jianyun8023 commented Aug 12, 2020

Can I not pass AsyncResponse to the base class? Make internalSetMaxUnackedMessagesOnSubscription return CompletableFuture<Void>. Other methods are like this.
You can refer to modify the use of asyncResponse
I think this can the base class and make it cleaner. en, I am going to post an issue description.

@codelipenghui What do you think?

@codelipenghui
Copy link
Contributor

@jianyun8023 Sound good to me, now I think there are many places pass the AsyncResponse. Since this is not a block problem, we can onboard this PR first and refine it later. You can create an issue for tracking this task.

Copy link
Contributor

@jianyun8023 jianyun8023 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, great.

@codelipenghui codelipenghui merged commit 9c694e5 into apache:master Aug 12, 2020
@codelipenghui codelipenghui mentioned this pull request Aug 12, 2020
14 tasks
@315157973 315157973 deleted the MaxUnackPerSubscription branch August 13, 2020 02:32
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
Motivation
support set MaxUnackMessagesPerSubscription on topic level

Modifications
Support set/get/remove MaxUnackMessagesPerSubscription policy on topic level.

Verifying this change
Added Unit test to verify set/get/remove MaxUnackMessagesPerSubscription policy at Topic level work as expected when Topic level policy is enabled/disabled

org.apache.pulsar.broker.admin.MaxUnackedMessagesTest#testMaxUnackedMessagesOnSubscriptionApi
org.apache.pulsar.broker.admin.MaxUnackedMessagesTest#testMaxUnackedMessagesOnSubscription
codelipenghui pushed a commit that referenced this pull request Sep 3, 2020
### Motivation
PR #7818 #7802 supports topic-level policies.
But the pulsar admin cli java doc is not supported accordingly.
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
Motivation
support set MaxUnackMessagesPerSubscription on topic level

Modifications
Support set/get/remove MaxUnackMessagesPerSubscription policy on topic level.

Verifying this change
Added Unit test to verify set/get/remove MaxUnackMessagesPerSubscription policy at Topic level work as expected when Topic level policy is enabled/disabled

org.apache.pulsar.broker.admin.MaxUnackedMessagesTest#testMaxUnackedMessagesOnSubscriptionApi
org.apache.pulsar.broker.admin.MaxUnackedMessagesTest#testMaxUnackedMessagesOnSubscription
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
### Motivation
PR apache#7818 apache#7802 supports topic-level policies.
But the pulsar admin cli java doc is not supported accordingly.
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
Motivation
support set MaxUnackMessagesPerSubscription on topic level

Modifications
Support set/get/remove MaxUnackMessagesPerSubscription policy on topic level.

Verifying this change
Added Unit test to verify set/get/remove MaxUnackMessagesPerSubscription policy at Topic level work as expected when Topic level policy is enabled/disabled

org.apache.pulsar.broker.admin.MaxUnackedMessagesTest#testMaxUnackedMessagesOnSubscriptionApi
org.apache.pulsar.broker.admin.MaxUnackedMessagesTest#testMaxUnackedMessagesOnSubscription
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
### Motivation
PR apache#7818 apache#7802 supports topic-level policies.
But the pulsar admin cli java doc is not supported accordingly.
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
Motivation
support set MaxUnackMessagesPerSubscription on topic level

Modifications
Support set/get/remove MaxUnackMessagesPerSubscription policy on topic level.

Verifying this change
Added Unit test to verify set/get/remove MaxUnackMessagesPerSubscription policy at Topic level work as expected when Topic level policy is enabled/disabled

org.apache.pulsar.broker.admin.MaxUnackedMessagesTest#testMaxUnackedMessagesOnSubscriptionApi
org.apache.pulsar.broker.admin.MaxUnackedMessagesTest#testMaxUnackedMessagesOnSubscription
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
### Motivation
PR apache#7818 apache#7802 supports topic-level policies.
But the pulsar admin cli java doc is not supported accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants