Skip to content

[Issue 2688]Support set retention on topic level. - #7747

Merged
codelipenghui merged 7 commits into
apache:masterfrom
jianyun8023:retention-on-topic-level
Aug 11, 2020
Merged

[Issue 2688]Support set retention on topic level.#7747
codelipenghui merged 7 commits into
apache:masterfrom
jianyun8023:retention-on-topic-level

Conversation

@jianyun8023

@jianyun8023 jianyun8023 commented Aug 4, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation

Support set retention quota on topic level.
Based on the system topic function.

Modifications

Support get-retention on topic level.
Support set-retention on topic level.
Support remove-retention on topic level.

Verifying this change

This change added tests and can be verified as follows:

  • test set topic retention
  • test remove topic retention
  • test get topic retention
  • test disabled topic retention
  • test topic retention parameters are valid

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: (don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes)
  • Anything that affects deployment: (no)

Documentation

  • Does this pull request introduce a new feature? (yes)
  • If yes, how is the feature documented? (docs / JavaDocs)

@codelipenghui codelipenghui added this to the 2.7.0 milestone Aug 5, 2020
@codelipenghui codelipenghui added the type/feature The PR added a new feature or issue requested a new feature label Aug 5, 2020
@jianyun8023
jianyun8023 force-pushed the retention-on-topic-level branch from 1fdcd11 to e224607 Compare August 5, 2020 02:10
@jianyun8023

jianyun8023 commented Aug 5, 2020

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

2 similar comments
@jianyun8023

jianyun8023 commented Aug 5, 2020

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@jianyun8023

jianyun8023 commented Aug 5, 2020

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui

codelipenghui commented Aug 5, 2020

Copy link
Copy Markdown
Contributor

@MarvinCai Could you please also help review this PR?

@codelipenghui codelipenghui mentioned this pull request Aug 5, 2020
Closed
14 tasks
@jianyun8023

jianyun8023 commented Aug 5, 2020

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@jianyun8023
jianyun8023 force-pushed the retention-on-topic-level branch from e224607 to e64936a Compare August 5, 2020 15:04
@jianyun8023

jianyun8023 commented Aug 5, 2020

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

MarvinCai
MarvinCai reviewed Aug 6, 2020
View reviewed changes

@MarvinCai MarvinCai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

left some minor comments

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
Comment thread pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/TopicPoliciesTest.java
Comment thread pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java Outdated
Comment thread pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java
@jianyun8023
jianyun8023 force-pushed the retention-on-topic-level branch from e64936a to dbd7771 Compare August 6, 2020 09:55
@jianyun8023

jianyun8023 commented Aug 6, 2020

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@sijie sijie mentioned this pull request Aug 10, 2020
Closed
13 tasks
codelipenghui
codelipenghui reviewed Aug 10, 2020
View reviewed changes

@codelipenghui codelipenghui left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me, just left some minor comments.

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
@jianyun8023
jianyun8023 force-pushed the retention-on-topic-level branch from fbfcdf2 to bebd6f7 Compare August 10, 2020 13:21
@jianyun8023

jianyun8023 commented Aug 10, 2020

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui
codelipenghui merged commit 824cb08 into apache:master Aug 11, 2020
zhanghaou
zhanghaou reviewed Aug 13, 2020
View reviewed changes

@zhanghaou zhanghaou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add some minor comments.

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
if (topicName.isGlobal()) {
validateGlobalNamespaceOwnership(namespaceName);
}
checkTopicLevelPolicyEnable();

@zhanghaou zhanghaou Aug 13, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

checkTopicLevelPolicyEnable() is also called by getTopicPolicies(), so we can delete it here.

@jianyun8023 jianyun8023 Aug 19, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

okay, let me solve these problems

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
if (topicName.isGlobal()) {
validateGlobalNamespaceOwnership(namespaceName);
}
checkTopicLevelPolicyEnable();

@zhanghaou zhanghaou Aug 13, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same to the above.

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
if (topicName.isGlobal()) {
validateGlobalNamespaceOwnership(namespaceName);
}
checkTopicLevelPolicyEnable();

@zhanghaou zhanghaou Aug 13, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same to the above.

Comment thread pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
void setRetention(String topic, RetentionPolicies retention) throws PulsarAdminException;

/**
* Set the retention configuration for all the topics on a topic asynchronously.

@zhanghaou zhanghaou Aug 13, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Need to modify this doc.

Comment thread pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
CompletableFuture<RetentionPolicies> getRetentionAsync(String topic);

/**
* Remove the retention configuration for all the topics on a topic.

@zhanghaou zhanghaou Aug 13, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Need to modify this doc.

huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
@jianyun8023 @huangdx0726
[Issue 2688]Support set retention on topic level. (apache#7747)
1a7c601 
### Motivation
Support set retention quota on topic level.
Based on the system topic function.
### Modifications
Support get-retention on topic level.
Support set-retention on topic level.
Support remove-retention on topic level.
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
@jianyun8023 Livio
[Issue 2688]Support set retention on topic level. (apache#7747)
00c7d6f 
### Motivation
Support set retention quota on topic level.
Based on the system topic function.
### Modifications
Support get-retention on topic level.
Support set-retention on topic level.
Support remove-retention on topic level.
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
@jianyun8023 Livio
[Issue 2688]Support set retention on topic level. (apache#7747)
894da0e 
### Motivation
Support set retention quota on topic level.
Based on the system topic function.
### Modifications
Support get-retention on topic level.
Support set-retention on topic level.
Support remove-retention on topic level.
lbenc135 pushed a commit to lbenc135/pulsar that referenced this pull request Sep 5, 2020
@jianyun8023 Livio
[Issue 2688]Support set retention on topic level. (apache#7747)
4cd719e 
### Motivation
Support set retention quota on topic level.
Based on the system topic function.
### Modifications
Support get-retention on topic level.
Support set-retention on topic level.
Support remove-retention on topic level.
@jianyun8023
jianyun8023 deleted the retention-on-topic-level branch April 15, 2022 01:08
@gaoran10 gaoran10 mentioned this pull request Jun 28, 2023
Merged
4 tasks
AlvaroStream added a commit to AlvaroStream/pulsar-site that referenced this pull request Jun 28, 2023
@AlvaroStream
Correct version issue
747e874 
Not valid from 2.6.0 

Thanks @gaoran10

> We introduce topic-level policies from 2.6.0, refer to apache/pulsar#4955, but I think we support retention policies at the topic level from 2.7.0, refer to apache/pulsar#7747.

>Make the default value of the configuration topicLevelPoliciesEnabled as true from 2.11.0, refer to apache/pulsar#15619.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MarvinCai MarvinCai MarvinCai left review comments
@codelipenghui codelipenghui codelipenghui approved these changes
@sijie sijie Awaiting requested review from sijie
@murong00 murong00 Awaiting requested review from murong00
@jiazhai jiazhai Awaiting requested review from jiazhai
@merlimat merlimat Awaiting requested review from merlimat
+1 more reviewer
@zhanghaou zhanghaou zhanghaou left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@jianyun8023 jianyun8023

Labels

type/feature The PR added a new feature or issue requested a new feature

Projects

None yet

Milestone

2.7.0

Development

Successfully merging this pull request may close these issues.

4 participants

@jianyun8023 @codelipenghui @zhanghaou @MarvinCai