Skip to content

[improve][offload]keep topic/ns set-offload-policies consistent behavior logic#20646

Merged
tisonkun merged 6 commits intoapache:masterfrom
ethqunzhong:dev-qunzhong-offload-policy-update
Jun 29, 2023
Merged

[improve][offload]keep topic/ns set-offload-policies consistent behavior logic#20646
tisonkun merged 6 commits intoapache:masterfrom
ethqunzhong:dev-qunzhong-offload-policy-update

Conversation

@ethqunzhong
Copy link
Copy Markdown
Contributor

Motivation

The existing behavior logic of the namespaces/topics set offload-policies is inconsistent.

  1. The namespaces set-offload-policies does not support configuring the filesystem because the bucket options are set as required.

  2. cli options are different, and the behavior of supporting unit conversion is inconsistent. eg:

    For example, there are two admin API methods to configure offload policy for persistent://myprop/clust/ns1/ds1:
    a. namespaces set-offload-policies in cli commond set-offload-policies myprop/clust/ns1 -d s3 -r region -b bucket -e endpoint -mbs 32M -rbs 5M -oat 10M -oats 100 -oae 10s -orp tiered-storage-first

    b. topics set-offload-policies in cli commond set-offload-policies persistent://myprop/clust/ns1/ds1 -d s3 -r region -b bucket -e endpoint -m 33554432 -rb 5242880 -t 10485760 -ts 100 -dl 10000 -orp tiered-storage-first

    The command has the same effect, but their command options are different and whether they recognize parameter units
    is also different. This can easily confuse users.

Modifications

  1. Remove unnecessary condition bucket required=true in namespace#set-offload-policies.
  2. Compatible options for namespace/topic and standardized support for unit recognition

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by supplementary unit-test.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: ethqunzhong#7

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jun 25, 2023
@ethqunzhong ethqunzhong changed the title [improve][offload]keep topic/ns set-offload-policies behavior same [improve][offload]keep topic/ns set-offload-policies consistent behavior logic Jun 26, 2023
Copy link
Copy Markdown
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Perfect

+1

Copy link
Copy Markdown
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Great!

@ethqunzhong
Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@sunheyi6
Copy link
Copy Markdown
Contributor

Great!

@tisonkun How do you ensure the correctness of the pr? Did you run the code and test it yourself, or can you tell if there is a problem just by looking at the code? I don't quite understand, please explain

@tisonkun
Copy link
Copy Markdown
Member

correctness

This patch contains tests and the code is correct and straightforward - normal review process.

please explain

Please avoid an imperative tone. I don't have to explain it to you.

@ethqunzhong
Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

1 similar comment
@ethqunzhong
Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@tisonkun
Copy link
Copy Markdown
Member

Blocked by #20661.

@ethqunzhong please keep an eye on that issue before doing repeatedly retry.

@ethqunzhong
Copy link
Copy Markdown
Contributor Author

Blocked by #20661.

@ethqunzhong please keep an eye on that issue before doing repeatedly retry.

got

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 29, 2023

Codecov Report

❌ Patch coverage is 53.44828% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.06%. Comparing base (2b01f83) to head (2e637ea).
⚠️ Report is 1934 commits behind head on master.

Files with missing lines Patch % Lines
...in/java/org/apache/pulsar/admin/cli/CmdTopics.java 52.83% 10 Missing and 15 partials ⚠️
...ava/org/apache/pulsar/admin/cli/CmdNamespaces.java 60.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20646       +/-   ##
=============================================
+ Coverage     33.58%   73.06%   +39.48%     
- Complexity    12127    31991    +19864     
=============================================
  Files          1613     1867      +254     
  Lines        126241   138737    +12496     
  Branches      13770    15258     +1488     
=============================================
+ Hits          42396   101370    +58974     
+ Misses        78331    29313    -49018     
- Partials       5514     8054     +2540     
Flag Coverage Δ
inttests 24.23% <0.00%> (+0.04%) ⬆️
systests 25.01% <0.00%> (?)
unittests 72.34% <53.44%> (+40.30%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ava/org/apache/pulsar/admin/cli/CmdNamespaces.java 76.25% <60.00%> (+76.25%) ⬆️
...in/java/org/apache/pulsar/admin/cli/CmdTopics.java 78.94% <52.83%> (+78.94%) ⬆️

... and 1518 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tisonkun
Copy link
Copy Markdown
Member

Merging..

Thank you!

@tisonkun tisonkun merged commit 204905e into apache:master Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants