[FIXED] Race condition in some leader failover scenarios leading to messages being potentially sourced more than once. #4592
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jnmoyne
force-pushed
the
jnm/fix_27Sept23
branch
from
eedcf9c to
f273609
Compare
|
We going to pull this in for 2.10.2? |
jnmoyne
force-pushed
the
jnm/fix_27Sept23
branch
3 times, most recently
from
625dd00 to
9d3136c
Compare
derekcollison
requested changes
Sep 27, 2023
jnmoyne
force-pushed
the
jnm/fix_27Sept23
branch
from
9d3136c to
96cfebe
Compare
…essages being potentially sourced more than once. - In some failure scenarios where the current leader of a stream sourcing from other stream(s) gets shutdown while publications are happening on the stream(s) being sourced leads to `setLeader(true)` being called on the new leader for the sourcing stream before all the messages having been sourced by the previous leader are completely processed such that when the new leader does it's reverse scan from the last message in it's view of the stream in order to know what sequence number to start the consumer for the stream being sourced from, such that the last message(s) sourced by the previous leader get sourced again, leading to some messages being sourced more than once. The existing `TestNoRaceJetStreamSuperClusterSources` test would sidestep the issue by relying on the deduplication window in the sourcing stream. Without deduplication the test is a flapper. This avoid the race condition by adding a small delay before scanning for the last message(s) having been sourced and starting the sources' consumer(s). Now the test (without using the deduplication window) never fails because more messages than expected have been received in the sourcing stream. - Fix test TestJetStreamWorkQueueSourceRestart that expects the sourcing stream to get all of the expected messages right away by adding a small sleep before checking the number of messages pending on the consumer for that stream. Signed-off-by: Jean-Noël Moyne <jnmoyne@gmail.com>
jnmoyne
force-pushed
the
jnm/fix_27Sept23
branch
from
96cfebe to
71f9688
Compare
|
If this runs green are we good to pull in? |
|
Yes |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
git pull --rebase origin main)Changes proposed in this pull request:
Fixes a race condition in some leader failover scenarios leading to messages being potentially sourced more than once.
In some failure scenarios where the current leader of a stream sourcing from other stream(s) gets shutdown while publications are happening on the stream(s) being sourced leads to
setLeader(true)being called on the new leader for the sourcing stream before all the messages having been sourced by the previous leader are completely processed such that when the new leader does it's reverse scan from the last message in it's view of the stream in order to know what sequence number to start the consumer for the stream being sourced from, such that the last message(s) sourced by the previous leader get sourced again, leading to some messages being sourced more than once.The existing
TestNoRaceJetStreamSuperClusterSourcestest would sidestep the issue by relying on the deduplication window in the sourcing stream. Without deduplication the test is a flapper.This avoid the race condition by adding a small delay before scanning for the last message(s) having been sourced and starting the sources' consumer(s). Now the test (without using the deduplication window) never fails because more messages than expected have been received in the sourcing stream.
(Also adds a guard to give up if
setupSourceConsumers()is called and we are no longer the leader for the stream (that check was already present insetupMirrorConsumer()so assuming it was forgotten forsetupSourceConsumers())