You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "poorbarcode (via GitHub)" <gi...@apache.org> on 2024/11/11 15:35:37 UTC
[PR] [fix] [broker] Do not take effect pub&sub rate-limit for system topics [pulsar]
poorbarcode opened a new pull request, #23589:
URL: https://github.com/apache/pulsar/pull/23589
### Motivation
The effect is huge if the `pub&sub` rate limiter takes effect for system topics, such as `__change_events`, transaction topics... Almost all topics can not work if the system topic encounters a throttling
### Modifications
- Do not take effect pub&sub rate-limit for system topics
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `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: x
--
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
Re: [PR] [improve][broker] Exclude system topics from namespace level publish and dispatch rate limiting [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode merged PR #23589:
URL: https://github.com/apache/pulsar/pull/23589
--
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
Re: [PR] [improve][broker] Exclude system topics from namespace level publish and dispatch rate limiting [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #23589:
URL: https://github.com/apache/pulsar/pull/23589#issuecomment-2470036132
> The only possible concern in a multi-tenant system is that a malicious user could circumvent rate limits by using a topic name that gets considered as a system topic. I guess this is a matter of the definition of the security model in the multi-tenancy support that rate limits cannot be used to protect against malicious users. We don't currently document the security model so it's hard to resolve this as part of this PR. I guess we could create a separate issue for documenting the security model for multi-tenancy.
Seems we never focused on the isolation for multi-tenants before, that should be a huge PIP, which contains Transaction(all namespaces use the same Transaction metadata store) and other components. 😂
Thanks for mentioning this, let me merge the PR first
--
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
Re: [PR] [fix] [broker] Do not take effect pub&sub rate-limit for system topics [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23589:
URL: https://github.com/apache/pulsar/pull/23589#issuecomment-2468624043
The only possible concern in a multi-tenant system is that a malicious user could circumvent rate-limits by using a topic name that gets considered as a system topic. I guess this is a matter of the definition of the security model in the multi-tenancy support that rate limits cannot be used to protect against malicious users. We don't currently document the security model so it's hard to resolve this as part of this PR. I guess we could create a separate issue for documenting the security model for multi-tenancy.
--
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
Re: [PR] [improve][broker] Exclude system topics from namespace level publish and dispatch rate limiting [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #23589:
URL: https://github.com/apache/pulsar/pull/23589#issuecomment-2469528774
/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
Re: [PR] [improve][broker] Exclude system topics from namespace level publish and dispatch rate limiting [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23589:
URL: https://github.com/apache/pulsar/pull/23589#issuecomment-2469765289
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23589?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `80.00000%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 74.31%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`4564f67`)](https://app.codecov.io/gh/apache/pulsar/commit/4564f67567e90cee70d7199b4d9accbd9f32fc9a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 723 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/23589?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...sar/broker/service/DisabledPublishRateLimiter.java](https://app.codecov.io/gh/apache/pulsar/pull/23589?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2FDisabledPublishRateLimiter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Rpc2FibGVkUHVibGlzaFJhdGVMaW1pdGVyLmphdmE=) | 50.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/23589?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/23589?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #23589 +/- ##
============================================
+ Coverage 73.57% 74.31% +0.73%
- Complexity 32624 34436 +1812
============================================
Files 1877 1944 +67
Lines 139502 147080 +7578
Branches 15299 16216 +917
============================================
+ Hits 102638 109299 +6661
- Misses 28908 29354 +446
- Partials 7956 8427 +471
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23589/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [inttests](https://app.codecov.io/gh/apache/pulsar/pull/23589/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.31% <80.00%> (+2.73%)` | :arrow_up: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/23589/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.35% <80.00%> (+0.03%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23589/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.70% <80.00%> (+0.86%)` | :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.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/23589?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...rg/apache/pulsar/broker/service/AbstractTopic.java](https://app.codecov.io/gh/apache/pulsar/pull/23589?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2FAbstractTopic.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Fic3RyYWN0VG9waWMuamF2YQ==) | `87.93% <100.00%> (-0.06%)` | :arrow_down: |
| [.../pulsar/broker/service/persistent/SystemTopic.java](https://app.codecov.io/gh/apache/pulsar/pull/23589?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2Fpersistent%2FSystemTopic.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvU3lzdGVtVG9waWMuamF2YQ==) | `84.21% <100.00%> (+4.21%)` | :arrow_up: |
| [...sar/broker/service/DisabledPublishRateLimiter.java](https://app.codecov.io/gh/apache/pulsar/pull/23589?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2FDisabledPublishRateLimiter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Rpc2FibGVkUHVibGlzaFJhdGVMaW1pdGVyLmphdmE=) | `50.00% <50.00%> (ø)` | |
... and [653 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/23589/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
--
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