[fix] [broker] In Key_Shared mode: remove unnecessary mechanisms of message skip to avoid unnecessary consumption stuck #20335
Conversation
…isms to avoid unnecessary blocking
poorbarcode
requested review from
Technoboy-,
equanz,
codelipenghui,
gaoran10 and
mattisonchao
There was a problem hiding this comment.
LGTM
I'll remove duplicated changes from #20179 when this PR is merged. (By the way, if you could, please check the new comments #20179 (comment) ?)
Sorry, I've been so busy lately. I left a comment to you #20179 (comment) |
…essage skip to avoid unnecessary consumption stuck (#20335) - #7105 provide a mechanism to avoid a stuck consumer affecting the consumption of other consumers: - if all consumers can not accept more messages, stop delivering messages to the client. - if one consumer can not accept more messages, just read new messages and deliver them to other consumers. - #7553 provide a mechanism to fix the issue of lost order of consumption: If the consumer cannot accept any more messages, skip the consumer for the next round of message delivery because there may be messages with the same key in the replay queue. - #10762 provide a mechanism to fix the issue of lost order of consumption: If there have any messages with the same key in the replay queue, do not deliver the new messages to this consumer. #10762 and #7553 do the same thing and #10762 is better than #7553 , so #7553 is unnecessary. remove the mechanism provided by #7553 to avoid unnecessary consumption stuck. (cherry picked from commit 1e664b7)
|
@poorbarcode just curious to know if the logic that was removed in this PR was causing problems. Is this PR an optimization or a bug fix? |
I think it should be called a fix.
If there have two consumers |
@poorbarcode Thanks for providing these details. I added the respective label. Please also update the PR description to reflect the fact that this is a bug fix.
Do you have a way to reproduce the problem and test that the fix is effective? Would you also be able to share those details? It would be useful to improve the unit & integration tests to cover such issues. I understand that the scaffolding for such tests is limited and I'm not a great fan of excessive mocking for simulating scenarios. However, providing more context for others in the community would be helpful. |
|
@codelipenghui @massakam do you have a chance to do a post-merge review since you have worked a lot on Key_Shared? |
…essage skip to avoid unnecessary consumption stuck (#20335) ### Motivation - #7105 provide a mechanism to avoid a stuck consumer affecting the consumption of other consumers: - if all consumers can not accept more messages, stop delivering messages to the client. - if one consumer can not accept more messages, just read new messages and deliver them to other consumers. - #7553 provide a mechanism to fix the issue of lost order of consumption: If the consumer cannot accept any more messages, skip the consumer for the next round of message delivery because there may be messages with the same key in the replay queue. - #10762 provide a mechanism to fix the issue of lost order of consumption: If there have any messages with the same key in the replay queue, do not deliver the new messages to this consumer. #10762 and #7553 do the same thing and #10762 is better than #7553 , so #7553 is unnecessary. ### Modifications remove the mechanism provided by #7553 to avoid unnecessary consumption stuck.
@codelipenghui @poorbarcode the part I'm wondering is the lack of tests for reproducing the problem that this PR intends to fix. Is there a way to add a test for this PR? /cc @Technoboy- |
…essage skip to avoid unnecessary consumption stuck (apache#20335) - apache#7105 provide a mechanism to avoid a stuck consumer affecting the consumption of other consumers: - if all consumers can not accept more messages, stop delivering messages to the client. - if one consumer can not accept more messages, just read new messages and deliver them to other consumers. - apache#7553 provide a mechanism to fix the issue of lost order of consumption: If the consumer cannot accept any more messages, skip the consumer for the next round of message delivery because there may be messages with the same key in the replay queue. - apache#10762 provide a mechanism to fix the issue of lost order of consumption: If there have any messages with the same key in the replay queue, do not deliver the new messages to this consumer. apache#10762 and apache#7553 do the same thing and apache#10762 is better than apache#7553 , so apache#7553 is unnecessary. remove the mechanism provided by apache#7553 to avoid unnecessary consumption stuck. (cherry picked from commit 1e664b7) (cherry picked from commit c973603)
I will do this, but it is not easy to reproduce in test. So will not soon. |
…essage skip to avoid unnecessary consumption stuck (#20335) ### Motivation - #7105 provide a mechanism to avoid a stuck consumer affecting the consumption of other consumers: - if all consumers can not accept more messages, stop delivering messages to the client. - if one consumer can not accept more messages, just read new messages and deliver them to other consumers. - #7553 provide a mechanism to fix the issue of lost order of consumption: If the consumer cannot accept any more messages, skip the consumer for the next round of message delivery because there may be messages with the same key in the replay queue. - #10762 provide a mechanism to fix the issue of lost order of consumption: If there have any messages with the same key in the replay queue, do not deliver the new messages to this consumer. #10762 and #7553 do the same thing and #10762 is better than #7553 , so #7553 is unnecessary. ### Modifications remove the mechanism provided by #7553 to avoid unnecessary consumption stuck. (cherry picked from commit 1e664b7)
Motivation
#10762 and #7553 do the same thing and #10762 is better than #7553 , so #7553 is unnecessary.
Modifications
remove the mechanism provided by #7553 to avoid unnecessary consumption stuck.
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: