Skip to content

[improve][admin]namespace CLI set-offload-policy's thresholdBytes support negative and 0 - #17502

Merged
Technoboy- merged 7 commits into
apache:masterfrom
ethqunzhong:set-namespace-offload-policy-threshold-support-negative
Sep 21, 2022
Merged

[improve][admin]namespace CLI set-offload-policy's thresholdBytes support negative and 0#17502
Technoboy- merged 7 commits into
apache:masterfrom
ethqunzhong:set-namespace-offload-policy-threshold-support-negative

Conversation

@ethqunzhong

Copy link
Copy Markdown
Contributor

Motivation

we could set namespace level offload-policy's offload-threshold In the following two pulsar-admin CLI ways:

  1. pulsar-admin namespaces set-offload-threshold -s $OFFLOAD_THRESHOLD demo/demo
  2. pulsar-admin namespaces set-offload-policies --offloadAfterThreshold $OFFLOAD_THRESHOLD demo/demo

while set offload-threshold value , Negative values disable automatic offload. 0 triggers offloading as soon as possible.
but we can't use pulsar-admin namespaces set-offload-policies to set offload-threshold'value to negative or 0 , show as follow:
image
pulsar-admin namespaces set-offload-threshold as expected.

image

we should keep set-offload-threshold and set-offload-policies CLI behavior consistent.

Modifications

remove positiveCheck process while check offloadAfterThreshold value validity at CmdNamespaces#SetOffloadPolicies.

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

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@ethqunzhong

Copy link
Copy Markdown
Contributor Author

@Technoboy- @zymap @hangc0276 PTAL.

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Sep 7, 2022
@ethqunzhong ethqunzhong changed the title namespace CLI set-offload-policy's thresholdBytes support negative and 0 [improve][admin]namespace CLI set-offload-policy's thresholdBytes support negative and 0 Sep 7, 2022
@zymap zymap added this to the 2.12.0 milestone Sep 7, 2022
@ethqunzhong

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@ethqunzhong

Copy link
Copy Markdown
Contributor Author

@hangc0276 @zymap @Technoboy- This patch is ready to merge. PTAL.

@ethqunzhong

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@ethqunzhong
ethqunzhong requested review from zymap and removed request for Technoboy- and hangc0276 September 8, 2022 06:12
@ethqunzhong

Copy link
Copy Markdown
Contributor Author

it seems pass all CI, ready to merged. PTAL @zymap
image

@ethqunzhong

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@ethqunzhong
ethqunzhong requested review from Technoboy- and removed request for zymap September 13, 2022 12:12
@ethqunzhong

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@ethqunzhong

Copy link
Copy Markdown
Contributor Author

@Technoboy- ready to merged. PTAL

@ethqunzhong

Copy link
Copy Markdown
Contributor Author

@codelipenghui ready to merged. PTAL

@Jason918 Jason918 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.

LGTM

@hangc0276 hangc0276 closed this Sep 21, 2022
@hangc0276 hangc0276 reopened this Sep 21, 2022
@Technoboy-
Technoboy- merged commit 677f24a into apache:master Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/admin doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants