Skip to content

[fix][client-tool]fix the topic offload policy param will be covered unexpected. #16357

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[fix][client-tool]fix the topic offload policy param will be covered unexpected. #16357

wants to merge 1 commit into from

Conversation

Nicklee007
Copy link
Contributor

@Nicklee007 Nicklee007 commented Jul 3, 2022 •
edited
Loading

Motivation

Compare the namespace offload policy, the topic offload policy command line update function use int and long type to accept param and miss some param check method, which will cause the param value set as 0 if the param is missed.

Modifications

  1. use String to accept param and add some check.
  2. Set a default value if the param is not set, and will check if is default value in PersistentTopicsBase.
  3. NamespacesBase and some check.

Documentation

  • doc-need

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 3, 2022
codelipenghui
codelipenghui reviewed Jul 4, 2022
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

Here is a good example #15657.

Could you please help add unit tests and update the documents?

@codelipenghui codelipenghui added this to the 2.11.0 milestone Jul 4, 2022
@codelipenghui codelipenghui added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker labels Jul 4, 2022
@Nicklee007 Nicklee007 force-pushed the fix-cmd-topic-policy-default-value branch from 435e31f to e5cbff3 Compare July 5, 2022 07:36
@Nicklee007 Nicklee007 requested a review from gaozhangmin July 7, 2022 10:05
@Nicklee007 Nicklee007 force-pushed the fix-cmd-topic-policy-default-value branch 2 times, most recently from 1dcbc12 to aa6b51f Compare July 20, 2022 07:13
@Nicklee007
Copy link
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022
@Nicklee007 Nicklee007 force-pushed the fix-cmd-topic-policy-default-value branch from aa6b51f to 8910eea Compare August 2, 2022 06:38
codelipenghui
codelipenghui reviewed Aug 8, 2022
View reviewed changes
site2/docs/reference-pulsar-admin.md

```bash

$ pulsar-admin topic set-offload-policies tenant/namespace/topic options
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$ pulsar-admin topic set-offload-policies tenant/namespace/topic options
$ pulsar-admin topics set-offload-policies tenant/namespace/topic options

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
Comment on lines +856 to +859
if (Objects.equals(offloadPolicies.getManagedLedgerOffloadDeletionLagInMillis(),
OffloadPoliciesImpl.DEFAULT_OFFLOAD_DELETION_LAG_IN_MILLIS)) {
offloadPolicies.setManagedLedgerOffloadDeletionLagInMillis(
currentOffloadPolicies.getManagedLedgerOffloadDeletionLagInMillis());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will change the behavior. After this change, users are not able to set the lagInMillis to null?

@github-actions
Copy link

github-actions bot commented Sep 8, 2022

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Sep 8, 2022
@poorbarcode
Copy link
Contributor

Since we will start the RC version of 3.0.0 on 2023-04-11, I will change the label/milestone of PR who have not been merged.

  • The PR of type feature is deferred to 3.1.0
  • The PR of type fix is deferred to 3.0.1

So drag this PR to 3.0.1

@poorbarcode poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023
@github-actions github-actions bot added doc-label-missing and removed doc-not-needed Your PR changes do not impact docs labels Apr 10, 2023
@github-actions
Copy link

@Nicklee007 Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@Technoboy- Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023
@Technoboy- Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023
@coderzc coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024
@lhotari lhotari modified the milestones: 4.0.0, 4.1.0 Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codelipenghui codelipenghui codelipenghui left review comments

@gaozhangmin gaozhangmin gaozhangmin approved these changes

Assignees

@Nicklee007 Nicklee007

Labels
area/broker doc-label-missing release/3.0.12 release/4.0.5 Stale type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.
8 participants
@Nicklee007 @poorbarcode @gaozhangmin @codelipenghui @lhotari @Technoboy- @RobertIndie @coderzc