(2.11) Don't send meta snapshot when becoming metaleader #5700
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
neilalexander
force-pushed
the
neil/jsmetasnap
branch
from
c6cf93d to
c20906a
Compare
neilalexander
force-pushed
the
neil/jsmetasnap
branch
from
c20906a to
5549baf
Compare
Antithesis testing has found that late or out-of-order delivery of these snapshots, likely due to latency or thread pauses, can cause stream assignments to be reverted which results in assets being deleted and recreated. There may also be a race condition where the metalayer comes up before network connectivity is fully established so we may end up generating snapshots that don't include assets we don't know about yet. We will want to audit all uses of `SendSnapshot` as it somewhat breaks the consistency model, especially now that we have fixed a significant number of Raft bugs that `SendSnapshot` usage may have been papering over. Further Antithesis runs without this code run fine and have eliminated a number of unexpected calls to `processStreamRemoval`. Signed-off-by: Neil Twigg <neil@nats.io>
neilalexander
force-pushed
the
neil/jsmetasnap
branch
from
5549baf to
e576cf1
Compare
Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
MauriceVanVeen
force-pushed
the
neil/jsmetasnap
branch
from
0858231 to
9ef2ed2
Compare
server/consumer.go
Outdated
| @@ -1703,7 +1703,7 @@ func (o *consumer) deleteNotActive() { | |||
| // Don't think this needs to be a monitored go routine. | |||
| go func() { | |||
| const ( | |||
| startInterval = 30 * time.Second | |||
| startInterval = 5 * time.Second | |||
There was a problem hiding this comment.
Why is this being changed in a PR about meta snapshots?
There was a problem hiding this comment.
Sending the snapshot was forcing the ghost consumers tests to pass for the wrong reason.
When we removed it, the test started to flake unless it was given over a minute to run. Reducing the initial time to clean up those ghost consumers de-flaked those tests without having to extend the test to such a long runtime.
There was a problem hiding this comment.
Yes but that can dramatically impact production systems with 10s or 100s of thousands of these. Hence the 30s vs shorter.
If need be make a var off of a const and set var to what you need in test and reset at the end of the test.
There was a problem hiding this comment.
OK will do tomorrow.
There was a problem hiding this comment.
In fact have just done now, as it was a quick change.
There was a problem hiding this comment.
Just removed the accidental const () that was left over too. Editor language server got me.
neilalexander
force-pushed
the
neil/jsmetasnap
branch
from
b45f105 to
9109439
Compare
Signed-off-by: Neil Twigg <neil@nats.io> Co-authored-by: Maurice van Veen <github@mauricevanveen.com>
neilalexander
force-pushed
the
neil/jsmetasnap
branch
from
9109439 to
03ed9c1
Compare
neilalexander
added a commit
that referenced
this pull request
Nov 19, 2024
Antithesis testing has found that late or out-of-order delivery of these snapshots, likely due to latency or thread pauses, can cause stream assignments to be reverted which results in assets being deleted and recreated. There may also be a race condition where the metalayer comes up before network connectivity to all other nodes is fully established so we may end up generating snapshots that don't include assets we don't know about yet. We will want to audit all uses of `SendSnapshot` as it somewhat breaks the consistency model, especially now that we have fixed a significant number of Raft bugs that `SendSnapshot` usage may have been papering over. Further Antithesis runs without this code run fine and have eliminated a number of unexpected calls to `processStreamRemoval`. We've also added a new unit test `TestJetStreamClusterHardKillAfterStreamAdd` for a long-known issue, as well as a couple tweaks to the ghost consumer tests to make them reliable. Signed-off-by: Neil Twigg <neil@nats.io> --------- Signed-off-by: Neil Twigg <neil@nats.io> Signed-off-by: Maurice van Veen <github@mauricevanveen.com> Co-authored-by: Maurice van Veen <github@mauricevanveen.com>
derekcollison
added a commit
that referenced
this pull request
Nov 20, 2024
We could have an empty apply queue length, but have stored uncommitted entries. If we then call `SendSnapshot` when becoming consumer leader we would be reverting back to previous state. This was also an issue for meta leader changes, which was fixed in #5700. This PR fixes it for consumer leader changes. Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
MauriceVanVeen
pushed a commit
that referenced
this pull request
Nov 21, 2024
We could have an empty apply queue length, but have stored uncommitted entries. If we then call `SendSnapshot` when becoming consumer leader we would be reverting back to previous state. This was also an issue for meta leader changes, which was fixed in #5700. This PR fixes it for consumer leader changes. Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
neilalexander
added a commit
that referenced
this pull request
Nov 22, 2024
Antithesis testing has found that late or out-of-order delivery of these snapshots, likely due to latency or thread pauses, can cause stream assignments to be reverted which results in assets being deleted and recreated. There may also be a race condition where the metalayer comes up before network connectivity to all other nodes is fully established so we may end up generating snapshots that don't include assets we don't know about yet. We will want to audit all uses of `SendSnapshot` as it somewhat breaks the consistency model, especially now that we have fixed a significant number of Raft bugs that `SendSnapshot` usage may have been papering over. Further Antithesis runs without this code run fine and have eliminated a number of unexpected calls to `processStreamRemoval`. We've also added a new unit test `TestJetStreamClusterHardKillAfterStreamAdd` for a long-known issue, as well as a couple tweaks to the ghost consumer tests to make them reliable. Signed-off-by: Neil Twigg <neil@nats.io> --------- Signed-off-by: Neil Twigg <neil@nats.io> Signed-off-by: Maurice van Veen <github@mauricevanveen.com> Co-authored-by: Maurice van Veen <github@mauricevanveen.com>
neilalexander
added a commit
that referenced
this pull request
Nov 25, 2024
Antithesis testing has found that late or out-of-order delivery of these snapshots, likely due to latency or thread pauses, can cause stream assignments to be reverted which results in assets being deleted and recreated. There may also be a race condition where the metalayer comes up before network connectivity to all other nodes is fully established so we may end up generating snapshots that don't include assets we don't know about yet. We will want to audit all uses of `SendSnapshot` as it somewhat breaks the consistency model, especially now that we have fixed a significant number of Raft bugs that `SendSnapshot` usage may have been papering over. Further Antithesis runs without this code run fine and have eliminated a number of unexpected calls to `processStreamRemoval`. We've also added a new unit test `TestJetStreamClusterHardKillAfterStreamAdd` for a long-known issue, as well as a couple tweaks to the ghost consumer tests to make them reliable. Signed-off-by: Neil Twigg <neil@nats.io> --------- Signed-off-by: Neil Twigg <neil@nats.io> Signed-off-by: Maurice van Veen <github@mauricevanveen.com> Co-authored-by: Maurice van Veen <github@mauricevanveen.com>
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.
Antithesis testing has found that late or out-of-order delivery of these snapshots, likely due to latency or thread pauses, can cause stream assignments to be reverted which results in assets being deleted and recreated. There may also be a race condition where the metalayer comes up before network connectivity to all other nodes is fully established so we may end up generating snapshots that don't include assets we don't know about yet.
We will want to audit all uses of
SendSnapshotas it somewhat breaks the consistency model, especially now that we have fixed a significant number of Raft bugs thatSendSnapshotusage may have been papering over.Further Antithesis runs without this code run fine and have eliminated a number of unexpected calls to
processStreamRemoval.We've also added a new unit test
TestJetStreamClusterHardKillAfterStreamAddfor a long-known issue, as well as a couple tweaks to the ghost consumer tests to make them reliable.Signed-off-by: Neil Twigg neil@nats.io