Knowden commented on code in PR #5171:
URL: https://github.com/apache/rocketmq/pull/5171#discussion_r978313890
##########
store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java:
##########
@@ -764,7 +764,7 @@ public GetMessageResult getMessage(final String group,
final String topic, final
boolean isInDisk =
checkInDiskByCommitOffset(offsetPy, maxOffsetPy);
- if (cqUnit.getQueueOffset() - offset >
maxFilterMessageCount) {
+ if ((cqUnit.getQueueOffset() - offset) *
ConsumeQueue.CQ_STORE_UNIT_SIZE > maxFilterMessageCount) {
Review Comment:
> For some pulled messages for maxMsgNums could be filter-out. so `Message
Count ` is not a good way to limit. Change Count-Limit to Bytes-Limit, you can
check code on 4.9.x.
>
>
https://github.com/apache/rocketmq/blob/c88686f13e6f4acef1f00c6d3e18589683ec3368/store/src/main/java/org/apache/rocketmq/store/DefaultMessageStore.java#L623
>
> but it seems need some updates on
>
> ```java
> if ((cqUnit.getQueueOffset() - offset) * CQ_STORE_UNIT_SIZE >
maxFilterMessageCount) {
> break;
> }
> ```
I'm agree with HScarb cause UNIT_SIZE will be different between
BatchConsumeQueue and ConsumeQueue. @francisoliverlee so I wander why we have
to use Byte-Limit rather than Count-Limit
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
