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.

Sign up for GitHub

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

Jump to bottom

Support get/remove/set retention policy command for topic #420

Merged
merged 1 commit into from
Aug 23, 2021
Merged

Support get/remove/set retention policy command for topic #420

merged 1 commit into from
Aug 23, 2021

Conversation

nodece
Copy link
Contributor

@nodece nodece commented Aug 12, 2021 •
edited
Loading

Signed-off-by: Zixuan Liu [email protected]

background from #246, the PR implements the following commands:

  • pulsarctl topics get-retention <topic> -a - Get the retention policy for a topic
  • pulsarctl topics remove-retention <topic> - Remove the retention policy for a topic
  • pulsarctl topics set-retention <topic> --time <string> --size <string> - Set the retention policy for a topic

@nodece nodece requested review from zymap and a team as code owners August 12, 2021 11:06
@nodece nodece changed the title support get/remove/set retention policy command for topic Support get/remove/set retention policy command for topic Aug 12, 2021
@nodece nodece force-pushed the retention_policy branch 2 times, most recently from 37c9072 to ab7f5d1 Compare August 13, 2021 06:02
zymap
zymap requested changes Aug 17, 2021
View reviewed changes
Copy link
Member

@zymap zymap left a comment

Choose a reason for hiding this comment

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

it's better to add a test to verify the whole process.
For example: set the retention to a topic and then get the value to verify is it set right. It can test both the cli and broker.

@nodece nodece force-pushed the retention_policy branch 3 times, most recently from 10e626e to 8970592 Compare August 18, 2021 02:28
@codelipenghui codelipenghui requested a review from zymap August 18, 2021 02:32
@nodece nodece force-pushed the retention_policy branch 2 times, most recently from 93d8dd0 to 9ff9573 Compare August 18, 2021 05:32
@nodece
Copy link
Contributor Author

nodece commented Aug 18, 2021

Hello @zymap, ut-tests always fail on Github Actions, but it is pass locally, could you give me some point?

zymap
zymap reviewed Aug 19, 2021
View reviewed changes
pkg/ctl/topic/set_retention.go Outdated Show resolved Hide resolved
pkg/ctl/topic/set_retention.go Outdated Show resolved Hide resolved
pkg/ctl/topic/set_retention.go Outdated Show resolved Hide resolved
pkg/ctl/topic/retention_test.go Outdated Show resolved Hide resolved
test/standalone.conf Outdated Show resolved Hide resolved
@zymap
Copy link
Member

zymap commented Aug 19, 2021 •
edited
Loading

@nodece I run the test on local as well and it failed. You can run a standalone and then run the test to check it.

@nodece nodece force-pushed the retention_policy branch 5 times, most recently from ab80ba7 to 625cafb Compare August 19, 2021 09:48
@nodece nodece requested a review from zymap August 19, 2021 10:10
@flowchartsman
Copy link
Contributor

@nodece I issued a PR against your PR branch to unify the tests and make them a bit like the dispatch tests that @limingnihao did (which I in turn based my publish-rate command and tests on). Hope it helps!

@nodece nodece force-pushed the retention_policy branch 2 times, most recently from 226df5b to 9d50c9c Compare August 20, 2021 06:15
@flowchartsman flowchartsman mentioned this pull request Aug 20, 2021
Closed
@zymap zymap merged commit c4edd49 into streamnative:master Aug 23, 2021
This was referenced Aug 23, 2021
Closed
Open
nodece added a commit that referenced this pull request Nov 12, 2021
@nodece
feat: add get/remove/set retention policy command for topic (#420)
eb59db6 
background from #246,  the PR implements the following commands:

- `pulsar topics get-retention <topic> -a` - Get the retention policy for a topic
- `pulsar topics remove-retention <topic>` - Remove the retention policy for a topic
- `pulsar topics set-retention <topic> --time <string> --size <string>` - Set the retention policy for a topic

(cherry picked from commit c4edd49)
tisonkun pushed a commit to tisonkun/pulsar-client-go that referenced this pull request Aug 15, 2023
@nodece @tisonkun
feat: add get/remove/set retention policy command for topic (streamna…
2680f18 
…tive/pulsarctl#420)

background from streamnative/pulsarctl#246,  the PR implements the following commands:

- `pulsar topics get-retention <topic> -a` - Get the retention policy for a topic
- `pulsar topics remove-retention <topic>` - Remove the retention policy for a topic
- `pulsar topics set-retention <topic> --time <string> --size <string>` - Set the retention policy for a topic
tisonkun pushed a commit to apache/pulsar-client-go that referenced this pull request Aug 16, 2023
@nodece @tisonkun
feat: add get/remove/set retention policy command for topic (streamna…
440de49 
…tive/pulsarctl#420)

background from streamnative/pulsarctl#246,  the PR implements the following commands:

- `pulsar topics get-retention <topic> -a` - Get the retention policy for a topic
- `pulsar topics remove-retention <topic>` - Remove the retention policy for a topic
- `pulsar topics set-retention <topic> --time <string> --size <string>` - Set the retention policy for a topic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zymap zymap zymap approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@limingnihao limingnihao Awaiting requested review from limingnihao

Assignees

@nodece nodece

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nodece @zymap @flowchartsman @codelipenghui