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/03/29 05:16:23 UTC
[PR] [fix][broker] Replication rate limiter counts batch messages [pulsar]
nodece opened a new pull request, #22380:
URL: https://github.com/apache/pulsar/pull/22380
### Motivation
The batch messages include multiple messages, but we only count 1.
### Modifications
If `msg.getMessageBuilder().hasNumMessagesInBatch()` returns true, use this value; otherwise, use 1.
### 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] Replication rate limiter counts batch messages [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #22380:
URL: https://github.com/apache/pulsar/pull/22380#issuecomment-2026801987
> I don't think that we should make this change since it's a breaking change. A better resolution is to document the behaviour. For replication, rate limiting entries rather than individual messages makes a lot of sense.
I understand that there is a breaking change here, but I want to keep the same way with the producer rate limit, please see https://github.com/lhotari/pulsar/blob/d61f7585e8ee1d5e76ff1ff1eb0b3380bc7178c4/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java#L324, we always use the batch size.
Right now, when using the batch messages, the `pulsar_replication_rate_in` and `pulsar_replication_rate_out` doesn't match.
pulsar_replication_rate_in > pulsar_replication_rate_out
--
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] Replication rate limiter counts batch messages [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat closed pull request #22380: [fix][broker] Replication rate limiter counts batch messages
URL: https://github.com/apache/pulsar/pull/22380
--
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] Replication rate limiter counts batch messages [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #22380:
URL: https://github.com/apache/pulsar/pull/22380#issuecomment-2029096158
/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] Replication rate limiter counts batch messages [pulsar]
Posted by "liudezhi2098 (via GitHub)" <gi...@apache.org>.
liudezhi2098 commented on PR #22380:
URL: https://github.com/apache/pulsar/pull/22380#issuecomment-2027104602
I also think it should be fixed and adapted to user habits.
--
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] Replication rate limiter counts batch messages [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #22380:
URL: https://github.com/apache/pulsar/pull/22380#issuecomment-2026819223
> I understand that there is a breaking change here, but I want to keep the same way with the producer rate limit, please see https://github.com/lhotari/pulsar/blob/d61f7585e8ee1d5e76ff1ff1eb0b3380bc7178c4/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java#L324, we always use the batch size.
I know that it feels wrong, but changing this is a breaking change. For rate limiting purposes, limiting the entry rate instead of message rate is useful and optimally it would be configurable. Entry rate limiting is more useful from the broker load perspective. The broker doesn't really care how many messages are batched in an entry. Rate limiting by message count matters mainly for consumers since the number of messages impact the load on client side message processing.
In the case of replication, that is not a concern at all.
I hope this reasoning explains why I think it's not a good idea to make this change.
>
> Right now, when using the batch messages, the `pulsar_replication_rate_in` and `pulsar_replication_rate_out` doesn't match.
>
> pulsar_replication_rate_in > pulsar_replication_rate_out
Isn't this a metrics issue? Can we fix this instead?
--
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] Replication rate limiter counts batch messages [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on PR #22380:
URL: https://github.com/apache/pulsar/pull/22380#issuecomment-2027070572
I think this should be a bug fix. I checked other places e.g. PersistentDispatcherMultipleConsumers, PersistentDispatcherSingleActiveConsumer, all of the other places are using messages, not batches. Not sure why only the replicator uses the batch.
IMO, we'd better keep it consistent to avoid confusion to users.
--
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] Replication rate limiter counts batch messages [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #22380:
URL: https://github.com/apache/pulsar/pull/22380#issuecomment-2026854789
> I know that it feels wrong, but changing this is a breaking change.
https://github.com/apache/pulsar/pull/4273/files#diff-16c47c755ed17a62606a6b9a6b1dc65d5fe0baed21f6e3541df264bec81b83f6R328 introduces rate limit for replication, perhaps not considering the batch messages.
We can discuss this on the dev mailing list.
> Rate limiting by message count matters mainly for consumers since the number of messages impact the load on client side message processing.
> In the case of replication, that is not a concern at all.
Producer rate limiting is important, and which impacts the consumer, but for some users who use the full-mesh replication, the users care about this.
When we use the batch size to request the token from the replication rate limiter, this will only reduce throughput, I think this is acceptable.
> Isn't this a metrics issue? Can we fix this instead?
`pulsar_replication_rate_out` from the local cluster, if an entry is a batch message, which includes multiple messages, but only records 1 msg by `msgOut.recordEvent(headersAndPayload.readableBytes());`.
`pulsar_replication_rate_in` from the remote cluster, when receiving this batch messages, which counts the how many messages in the batch messages.
so causes pulsar_replication_rate_in > pulsar_replication_rate_out
--
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] Replication rate limiter counts batch messages [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on PR #22380:
URL: https://github.com/apache/pulsar/pull/22380#issuecomment-6353165744
Thanks @nodece. The replicator on master still counts 1 msg per entry (`GeoPersistentReplicator` / `ShadowReplicator`), so the problem is real, but this branch no longer applies after the replicator rework (#26005, #26620) and the limiter semantics are still disputed. I'm closing it; the limiter part should be done together with #26959 (dispatch rate overshoot with batches): the replicator should honor `dispatchThrottlingOnBatchMessageEnabled` like the dispatchers and size its reads accordingly. A separate small PR that counts `msgOut`/OTel out-counters by batch size would be welcome to fix the `replication_rate_in` vs `rate_out` mismatch.
--
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