Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

There may be a bug in DefaultMessageStore#getMessage #5162

Closed
Knowden opened this issue Sep 22, 2022 · 5 comments · Fixed by #5171
Closed

There may be a bug in DefaultMessageStore#getMessage #5162

Knowden opened this issue Sep 22, 2022 · 5 comments · Fixed by #5171

Comments

@Knowden
Copy link
Contributor

Knowden commented Sep 22, 2022

maxFilterMessageCount is calculated as maxMsgNums * ConsumeQueue.CQ_STORE_UNIT_SIZE and it's seems like meaning the maxFilterMessageSize.

final int maxFilterMessageCount = Math.max(16000, maxMsgNums * ConsumeQueue.CQ_STORE_UNIT_SIZE);

and it's used to judge wether this pull has scanned enough message as follow

if (cqUnit.getQueueOffset() - offset > maxFilterMessageCount) {
    break;
}

the cqUnit.queueOffset is logic offset of consumer queue which is calculated as follow

long queueOffset = (sbr.getStartOffset() + sbr.getByteBuffer().position() -  relativePos) / CQ_STORE_UNIT_SIZE;
CqUnit cqUnit = new CqUnit(queueOffset,
        sbr.getByteBuffer().getLong(),
        sbr.getByteBuffer().getInt(),
        sbr.getByteBuffer().getLong());

Is there something going wrong? or if I made some mistake when reading source code and submitting issue, please figure it out thanks

@francisoliverlee
Copy link
Member

francisoliverlee commented Sep 22, 2022

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.

for (; i < bufferConsumeQueue.getSize() && i < maxFilterMessageCount; i += ConsumeQueue.CQ_STORE_UNIT_SIZE) {

but it seems need some updates on

if ((cqUnit.getQueueOffset() - offset) *  CQ_STORE_UNIT_SIZE  > maxFilterMessageCount) {
    break;
}

@HScarb
Copy link
Contributor

HScarb commented Sep 23, 2022

IMO you're right.
maxFilterMessageCount is physical offset of ConsumeQueue file, while cqUnit.getQueueOffset() - offset is logical offset

@Knowden
Copy link
Contributor Author

Knowden commented Sep 23, 2022

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.

for (; i < bufferConsumeQueue.getSize() && i < maxFilterMessageCount; i += ConsumeQueue.CQ_STORE_UNIT_SIZE) {

but it seems need some updates on

if ((cqUnit.getQueueOffset() - offset) *  CQ_STORE_UNIT_SIZE  > maxFilterMessageCount) {
    break;
}

ok. I will submit a PR to fix this later on.

@Knowden
Copy link
Contributor Author

Knowden commented Sep 23, 2022

#5171

@ni-ze
Copy link
Contributor

ni-ze commented Sep 26, 2022

LGTM~

RongtongJin pushed a commit that referenced this issue Sep 29, 2022
… calculating (#5171)

* fix bug for comparing size with count

* add getUnitSize function
drpmma pushed a commit that referenced this issue Feb 21, 2023
… calculating (#5171)

* fix bug for comparing size with count

* add getUnitSize function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants