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

QQ: fix bug when subscribing using an already existing consumer tag #9158

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

kjnilsson
Copy link
Contributor

@kjnilsson kjnilsson commented Aug 23, 2023

When subscribing using a consumer tag that is already in the quorum
queues state (but perhaps with a cancelled status) and that has
pending messages the next_msg_id which is used to initialise the
queue type consumer state did not take the in flight message ids into
account. This resulted in some messages occasionally not being delivered
to the clint and thus would appear stuck as awaiting acknowledgement
for the consumer.

When a new checkout operation detects there are in-flight messages
we set the last_msg_id to undefined and just accept the next message
that arrives, irrespective of their message id. This isn't 100% fool proof
as there may be cases where messages are lost between queue and channel
where we'd miss to trigger the fallback query for missing messages.

It is however much better than what we have atm.

NB: really the ideal solution would be to make checkout operations
async so that any inflight messages are delivered before the checkout
result. That is a much bigger change for another day.

Copy link
Contributor

@mkuratczyk mkuratczyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been running this patch for many hours and the issue hasn't occurred.
Without this PR, it occurred multiple times per minute.

@kjnilsson kjnilsson changed the title QQ: fix bug when subscribing using an already existing consumer tag DO NOT MERGE: QQ: fix bug when subscribing using an already existing consumer tag Aug 23, 2023
When subscribing using a consumer tag that is already in the quorum
queues state (but perhaps with a cancelled status) and that has
pending messages the next_msg_id which is used to initialise the
queue type consumer state did not take the in flight message ids into
account. This resulted in some messages occasionally not being delivered
to the clint and thus would appear stuck as awaiting acknowledgement
for the consumer.

When a new checkout operation detects there are in-flight messages
we set the last_msg_id to `undefined` and just accept the next message
that arrives, irrespective of their message id. This isn't 100% fool proof
as there may be cases where messages are lost between queue and channel
where we'd miss to trigger the fallback query for missing messages.

It is however much better than what we have atm.

NB: really the ideal solution would be to make checkout operations
async so that any inflight messages are delivered before the checkout
result. That is a much bigger change for another day.
@kjnilsson kjnilsson changed the title DO NOT MERGE: QQ: fix bug when subscribing using an already existing consumer tag QQ: fix bug when subscribing using an already existing consumer tag Aug 23, 2023
@mkuratczyk
Copy link
Contributor

mkuratczyk commented Aug 23, 2023

Cool, I've run multiple tests with the second version of the fix and could not reproduce neither missed nor duplicated messages.

@kjnilsson kjnilsson merged commit 395ee69 into main Aug 23, 2023
16 checks passed
@kjnilsson kjnilsson deleted the gh_9138 branch August 23, 2023 16:24
@michaelklishin michaelklishin added this to the 3.12.4 milestone Aug 23, 2023
michaelklishin added a commit that referenced this pull request Aug 23, 2023
QQ: fix bug when subscribing using an already existing consumer tag (backport #9158)
michaelklishin added a commit that referenced this pull request Aug 25, 2023
QQ: fix bug when subscribing using an already existing consumer tag (backport #9158) (backport #9161)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants