VStreamer: fix deadlock when there are a lot of vschema changes at the same time as binlog events #11325
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
…stream load Signed-off-by: Rohit Nayak <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
The VStreamer engine is somewhat unusual in two ways:
1. It is open and running on replica tablets rather than only
running on primary tablets.
2. It has no controllers so the main engine mutex is widely shared.
Because of this, when a tablet has open vstreams (direct binary log
streams) performing work and a state transition starts, it can
deadlock with the tabletmanager's state lock when checking if
the engine is open or not.
Signed-off-by: Matt Lord <[email protected]>
rohit-nayak-ps
requested review from
harshit-gangal,
systay and
shlomi-noach
as code owners
rohit-nayak-ps
changed the title
mattlord
approved these changes
Sep 28, 2022
|
|
||
| flags := &vtgatepb.VStreamFlags{} | ||
|
|
||
| ctx2, cancel := context.WithTimeout(ctx, 2*time.Minute) |
There was a problem hiding this comment.
This timeout is equal to the extendedTimeout var, right? Might as well use that IMO so that we can easily change timeouts in various places.
| "Args": ["vitess.io/vitess/go/test/endtoend/vreplication", "-run", "TestVSchemaChangesUnderLoad"], | ||
| "Command": [], | ||
| "Manual": false, | ||
| "Shard": "vreplication_cellalias", |
There was a problem hiding this comment.
We should probably rename the shard, e.g. vreplication_cellalias_vschema or something.
Signed-off-by: Rohit Nayak <[email protected]>
This was referenced Oct 5, 2022
This was referenced Oct 25, 2022
DeathBorn
pushed a commit
to vinted/vitess
that referenced
this pull request
Jan 26, 2023
VStreamer: fix deadlock when there are a lot of vschema changes at the same time as binlog events (vitessio#11325) * Don't block on vschema channel in case of heavy vschema changes and vstream load Signed-off-by: Rohit Nayak <[email protected]> * Prevent VStreamer engine deadlocks during state transitions The VStreamer engine is somewhat unusual in two ways: 1. It is open and running on replica tablets rather than only running on primary tablets. 2. It has no controllers so the main engine mutex is widely shared. Because of this, when a tablet has open vstreams (direct binary log streams) performing work and a state transition starts, it can deadlock with the tabletmanager's state lock when checking if the engine is open or not. Signed-off-by: Matt Lord <[email protected]> * Address review comments Signed-off-by: Rohit Nayak <[email protected]> Signed-off-by: Rohit Nayak <[email protected]> Signed-off-by: Matt Lord <[email protected]> Co-authored-by: Matt Lord <[email protected]> Signed-off-by: Vilius Okockis <[email protected]>
This was referenced May 22, 2023
This was referenced Jun 20, 2023
4 tasks
This was referenced Feb 2, 2024
This was referenced Feb 11, 2024
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.
Description
This fixes the bug reported in #11169
Pre-conditions for bug:
Bug cause:
vstreamer.SetVSchema()the new vschema is written to a single message buffered channel for each active vstreamer. This is read by the vstreamer event processor, holding the vstreamer engine mutex.casestatements: one reads from the vschema channel and other reads the binlog events. The binlog events are streamed to the receiver. If the receiver is not reading them then the send will block. This means that the othercasewhich polls the vschema change channel will not get called. This results in the vstreamer engine lock being held until all events are streamedThis PR drains the vschema channel so that only the latest vschema update is sent. So it no longer holds the vstreamer lock for an extended time allowing the PRS to proceed.
This PR also incorporates the changes made in #11268 which also solves the same issue in a different way: the vschema update no longer holds a lock (since it uses
atomic.int32to signal whether the vstreamer engine is open, rather than the mutex). #11268 also adds checks for cancelled contexts.Related Issue(s)
fixes #11169
Checklist