You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "ethqunzhong (via GitHub)" <gi...@apache.org> on 2023/06/25 13:04:59 UTC
[GitHub] [pulsar] ethqunzhong opened a new pull request, #20646: [improve][offload]keep topic/ns set-offload-policies behavior same
ethqunzhong opened a new pull request, #20646:
URL: https://github.com/apache/pulsar/pull/20646
### 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
- [x] 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
- [x] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: https://github.com/ethqunzhong/pulsar/pull/7
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shy-share commented on pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "shy-share (via GitHub)" <gi...@apache.org>.
shy-share commented on PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#issuecomment-1608505294
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#issuecomment-1608645336
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] ethqunzhong commented on pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "ethqunzhong (via GitHub)" <gi...@apache.org>.
ethqunzhong commented on PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#issuecomment-1610574464
> Blocked by #20661.
>
> @ethqunzhong please keep an eye on that issue before doing repeatedly retry.
got
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] ethqunzhong commented on pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "ethqunzhong (via GitHub)" <gi...@apache.org>.
ethqunzhong commented on PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#issuecomment-1607325920
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] JooHyukKim commented on a diff in pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "JooHyukKim (via GitHub)" <gi...@apache.org>.
JooHyukKim commented on code in PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#discussion_r1245918900
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java:
##########
@@ -2289,7 +2291,7 @@ private class SetOffloadPolicies extends CliCommand {
@Parameter(
names = {"--offloadAfterThresholdInSeconds", "-oats"},
- description = "Offload after threshold seconds (eg: 1,5,10)",
+ description = "Offload after threshold seconds (or minutes,hours,days,weeks eg: 100m, 3h, 2d, 5w).",
required = false
)
Review Comment:
True, I will make a follow up issue on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codecov-commenter commented on pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#issuecomment-1612403826
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/20646?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#20646](https://app.codecov.io/gh/apache/pulsar/pull/20646?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (2e637ea) into [master](https://app.codecov.io/gh/apache/pulsar/commit/2b01f83ea4f3ab7b22f4ea2b0290cc183e79bbb8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (2b01f83) will **increase** coverage by `39.48%`.
> The diff coverage is `53.44%`.
[](https://app.codecov.io/gh/apache/pulsar/pull/20646?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ 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%)` | :arrow_up: |
| systests | `25.01% <0.00%> (?)` | |
| unittests | `72.34% <53.44%> (+40.30%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pulsar/pull/20646?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...in/java/org/apache/pulsar/admin/cli/CmdTopics.java](https://app.codecov.io/gh/apache/pulsar/pull/20646?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC10b29scy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2FkbWluL2NsaS9DbWRUb3BpY3MuamF2YQ==) | `78.94% <52.83%> (+78.94%)` | :arrow_up: |
| [...ava/org/apache/pulsar/admin/cli/CmdNamespaces.java](https://app.codecov.io/gh/apache/pulsar/pull/20646?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC10b29scy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2FkbWluL2NsaS9DbWROYW1lc3BhY2VzLmphdmE=) | `76.25% <60.00%> (+76.25%)` | :arrow_up: |
... and [1518 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/20646/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] ethqunzhong commented on pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "ethqunzhong (via GitHub)" <gi...@apache.org>.
ethqunzhong commented on PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#issuecomment-1608867241
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#issuecomment-1612404734
Merging..
Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] JooHyukKim commented on a diff in pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "JooHyukKim (via GitHub)" <gi...@apache.org>.
JooHyukKim commented on code in PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#discussion_r1245276494
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java:
##########
@@ -2289,7 +2291,7 @@ private class SetOffloadPolicies extends CliCommand {
@Parameter(
names = {"--offloadAfterThresholdInSeconds", "-oats"},
- description = "Offload after threshold seconds (eg: 1,5,10)",
+ description = "Offload after threshold seconds (or minutes,hours,days,weeks eg: 100m, 3h, 2d, 5w).",
required = false
)
Review Comment:
I found a possibly neat solution to isolate parsing logic that eventually can be applied in so many ways,
```suggestion
description = "Offload after threshold seconds (or minutes,hours,days,weeks eg: 100m, 3h, 2d, 5w).",
required = false, converter = TimeUnitToSecondConverter.class)
)
```
or you can refer to the revisions in my PR https://github.com/apache/pulsar/pull/20663/files#r1245268820 for more reference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] JooHyukKim commented on a diff in pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "JooHyukKim (via GitHub)" <gi...@apache.org>.
JooHyukKim commented on code in PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#discussion_r1245276494
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java:
##########
@@ -2289,7 +2291,7 @@ private class SetOffloadPolicies extends CliCommand {
@Parameter(
names = {"--offloadAfterThresholdInSeconds", "-oats"},
- description = "Offload after threshold seconds (eg: 1,5,10)",
+ description = "Offload after threshold seconds (or minutes,hours,days,weeks eg: 100m, 3h, 2d, 5w).",
required = false
)
Review Comment:
I found a possibly neat solution to isolate parsing logic that eventually can be applied.
```suggestion
description = "Offload after threshold seconds (or minutes,hours,days,weeks eg: 100m, 3h, 2d, 5w).",
required = false, converter = TimeUnitToSecondConverter.class)
)
```
or you can refer to the revisions in my PR https://github.com/apache/pulsar/pull/20663/files#r1245268820 for more reference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#discussion_r1245906971
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java:
##########
@@ -2289,7 +2291,7 @@ private class SetOffloadPolicies extends CliCommand {
@Parameter(
names = {"--offloadAfterThresholdInSeconds", "-oats"},
- description = "Offload after threshold seconds (eg: 1,5,10)",
+ description = "Offload after threshold seconds (or minutes,hours,days,weeks eg: 100m, 3h, 2d, 5w).",
required = false
)
Review Comment:
Looks good. While this patch makes use of existing utils, I suggest we do a follow-up refactor to review that we can replace handmade utils with lib-provided methods. It wouldn't block this patch from merging, IMO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun merged pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun merged PR #20646:
URL: https://github.com/apache/pulsar/pull/20646
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#issuecomment-1610166273
Blocked by https://github.com/apache/pulsar/issues/20661.
@ethqunzhong please keep an eye on that issue before doing repeatedly retry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] ethqunzhong commented on pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "ethqunzhong (via GitHub)" <gi...@apache.org>.
ethqunzhong commented on PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#issuecomment-1608918607
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] ethqunzhong commented on pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "ethqunzhong (via GitHub)" <gi...@apache.org>.
ethqunzhong commented on PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#issuecomment-1608724879
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] JooHyukKim commented on a diff in pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "JooHyukKim (via GitHub)" <gi...@apache.org>.
JooHyukKim commented on code in PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#discussion_r1245918900
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java:
##########
@@ -2289,7 +2291,7 @@ private class SetOffloadPolicies extends CliCommand {
@Parameter(
names = {"--offloadAfterThresholdInSeconds", "-oats"},
- description = "Offload after threshold seconds (eg: 1,5,10)",
+ description = "Offload after threshold seconds (or minutes,hours,days,weeks eg: 100m, 3h, 2d, 5w).",
required = false
)
Review Comment:
True. No need to block this PR, I will make a follow up issue on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] ethqunzhong commented on a diff in pull request #20646: [improve][offload]keep topic/ns set-offload-policies consistent behavior logic
Posted by "ethqunzhong (via GitHub)" <gi...@apache.org>.
ethqunzhong commented on code in PR #20646:
URL: https://github.com/apache/pulsar/pull/20646#discussion_r1246009922
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java:
##########
@@ -2289,7 +2291,7 @@ private class SetOffloadPolicies extends CliCommand {
@Parameter(
names = {"--offloadAfterThresholdInSeconds", "-oats"},
- description = "Offload after threshold seconds (eg: 1,5,10)",
+ description = "Offload after threshold seconds (or minutes,hours,days,weeks eg: 100m, 3h, 2d, 5w).",
required = false
)
Review Comment:
>
@JooHyukKim Thank you for your suggestions. It's appreciated that you took the effort to add them in other PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org