You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "nodece (via GitHub)" <gi...@apache.org> on 2024/05/08 10:38:07 UTC
[PR] [Fix][broker] Limit replication rate based on bytes [pulsar]
nodece opened a new pull request, #22674:
URL: https://github.com/apache/pulsar/pull/22674
### Motivation
When the replicator rate limit value based on bytes is less than the `org.apache.pulsar.broker.service.persistent.PersistentReplicator#readMaxSizeBytes`, this will cause the outgoing bytes to exceed the limit value.
The goal is to make outgoing bytes as close to the rate limit value as possible.
### Modifications
- `org.apache.pulsar.broker.service.persistent.PersistentReplicator#getAvailablePermits` returns the available messages and bytes, and then reads the entries based on that.
### Verifying this change
The test has been added.
### 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 -->
--
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] Limit replication rate based on bytes [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #22674:
URL: https://github.com/apache/pulsar/pull/22674#issuecomment-2167094595
/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] [Fix][broker] Limit replication rate based on bytes [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #22674:
URL: https://github.com/apache/pulsar/pull/22674#issuecomment-2108724412
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/22674?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 `65.38462%` with `9 lines` in your changes are missing coverage. Please review.
> Project coverage is 73.18%. 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 [(`a3277c3`)](https://app.codecov.io/gh/apache/pulsar/pull/22674?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 264 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/22674?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #22674 +/- ##
============================================
- Coverage 73.57% 73.18% -0.40%
- Complexity 32624 32849 +225
============================================
Files 1877 1889 +12
Lines 139502 141370 +1868
Branches 15299 15515 +216
============================================
+ Hits 102638 103457 +819
- Misses 28908 29911 +1003
- Partials 7956 8002 +46
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/22674/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/22674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.52% <42.30%> (+2.93%)` | :arrow_up: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/22674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.70% <0.00%> (+0.38%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/22674/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.20% <65.38%> (-0.65%)` | :arrow_down: |
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](https://app.codecov.io/gh/apache/pulsar/pull/22674?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...roker/service/persistent/PersistentReplicator.java](https://app.codecov.io/gh/apache/pulsar/pull/22674?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2Fpersistent%2FPersistentReplicator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvUGVyc2lzdGVudFJlcGxpY2F0b3IuamF2YQ==) | `67.54% <65.38%> (-1.34%)` | :arrow_down: |
... and [353 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/22674/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
Re: [PR] [Fix][broker] Limit replication rate based on bytes [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #22674:
URL: https://github.com/apache/pulsar/pull/22674#discussion_r1601933591
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentReplicator.java:
##########
@@ -199,11 +199,11 @@ protected void disableReplicatorRead() {
* Calculate available permits for read entries.
*
* @return
- * 0: Producer queue is full, no permits.
- * -1: Rate Limiter reaches limit.
- * >0: available permits for read entries.
+ * 0, O: Producer queue is full, no permits.
+ * -1, -1: Rate Limiter reaches limit.
+ * >0, >0: available permits for read entries.
*/
- private int getAvailablePermits() {
+ public Pair<Integer, Long> getAvailablePermits() {
Review Comment:
Ping @lhotari
--
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] Limit replication rate based on bytes [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #22674:
URL: https://github.com/apache/pulsar/pull/22674#discussion_r1597340168
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentReplicator.java:
##########
@@ -199,11 +199,11 @@ protected void disableReplicatorRead() {
* Calculate available permits for read entries.
*
* @return
- * 0: Producer queue is full, no permits.
- * -1: Rate Limiter reaches limit.
- * >0: available permits for read entries.
+ * 0, O: Producer queue is full, no permits.
+ * -1, -1: Rate Limiter reaches limit.
+ * >0, >0: available permits for read entries.
*/
- private int getAvailablePermits() {
+ public Pair<Integer, Long> getAvailablePermits() {
Review Comment:
I can add a class to hold the available messages and bytes, so like:
```
@Data
@AllArgsConstructor
private static class AvailablePermits {
private int messages;
private long bytes;
}
```
`record` requires the JDK 14, we cannot cherry-pick this PR to the previous version.
--
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] Limit replication rate based on bytes [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #22674:
URL: https://github.com/apache/pulsar/pull/22674#issuecomment-2108567733
/pulsarbot rerun-failure
--
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] Limit replication rate based on bytes [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #22674:
URL: https://github.com/apache/pulsar/pull/22674#issuecomment-2108568614
/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] [Fix][broker] Limit replication rate based on bytes [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #22674:
URL: https://github.com/apache/pulsar/pull/22674#discussion_r1596930563
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentReplicator.java:
##########
@@ -199,11 +199,11 @@ protected void disableReplicatorRead() {
* Calculate available permits for read entries.
*
* @return
- * 0: Producer queue is full, no permits.
- * -1: Rate Limiter reaches limit.
- * >0: available permits for read entries.
+ * 0, O: Producer queue is full, no permits.
+ * -1, -1: Rate Limiter reaches limit.
+ * >0, >0: available permits for read entries.
*/
- private int getAvailablePermits() {
+ public Pair<Integer, Long> getAvailablePermits() {
Review Comment:
It's better to use a record than to use Pair. Pair results in hard-to-read code. An additional reason here is that Integer and Long are boxed objects which are adding additional overhead compared to a record.
--
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