Do not mutate replication state in WaitSourcePos and ignore tablets with SQL_Thread stopped in ERS #10148
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
This results in race conditions with tablet repairs that can leave replication broken and requiring manual repair using: START SLAVE. Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
…topped Signed-off-by: Manan Gupta <manan@planetscale.com>
GuptaManan100
force-pushed
the
ro_state_waitsourcepos
branch
from
315dbc0
to
3e128dd
Compare
3 tasks
mattlord
force-pushed
the
ro_state_waitsourcepos
branch
from
6a8ade8
to
3e128dd
Compare
…cation status Signed-off-by: Manan Gupta <manan@planetscale.com>
GuptaManan100
force-pushed
the
ro_state_waitsourcepos
branch
from
e4030b1
to
347b17e
Compare
…ich is correct Signed-off-by: Manan Gupta <manan@planetscale.com>
3 tasks
GuptaManan100
changed the title
mattlord
requested review from
ajm188,
doeg,
notfelineit and
deepthi
as code owners
GuptaManan100
approved these changes
Apr 29, 2022
mattlord
commented
Apr 29, 2022
| @@ -141,7 +141,7 @@ func TestEmergencyReparentShardSlow(t *testing.T) { | |||
| }, | |||
| "zone1-0000000200": { | |||
| StopStatus: &replicationdatapb.StopReplicationStatus{ | |||
| Before: &replicationdatapb.Status{}, | |||
| Before: &replicationdatapb.Status{IoState: int32(mysql.ReplicationStateRunning), SqlState: int32(mysql.ReplicationStateRunning)}, | |||
There was a problem hiding this comment.
We're changing this — so that it's correct — all over because we now use it in the tests. Before it was unused.
mattlord
commented
Apr 29, 2022
| } | ||
|
|
||
| replStatus := mysql.ProtoToReplicationStatus(stopStatus.Before) | ||
| return replStatus.SQLState == mysql.ReplicationStateRunning, nil |
There was a problem hiding this comment.
This will be backwards compatible once #10167 is merged.
mattlord
commented
Apr 29, 2022
Comment on lines
+299
to
+300
| Before: &replicationdatapb.Status{Position: "MySQL56/3E11FA47-71CA-11E1-9E33-C80AA9429100:1-5", IoState: int32(mysql.ReplicationStateRunning), SqlState: int32(mysql.ReplicationStateRunning)}, | ||
| After: &replicationdatapb.Status{Position: "MySQL56/3E11FA47-71CA-11E1-9E33-C80AA9429100:1-9"}, |
There was a problem hiding this comment.
Nice! I like the explicitness of it.
|
LGTM. Thanks, @GuptaManan100 ! |
deepthi
pushed a commit
that referenced
this pull request
Apr 29, 2022
…ith SQL_Thread stopped in ERS (#10148) - Release 13.0 (#10175) * Do not mutate replication state in WaitSourcePos This results in race conditions with tablet repairs that can leave replication broken and requiring manual repair using: START SLAVE. Signed-off-by: Matt Lord <mattalord@gmail.com> feat: throw error for tablets that have replication stopped in ERS Signed-off-by: Manan Gupta <manan@planetscale.com> test: fix reparent tests to reflect the new change Signed-off-by: Manan Gupta <manan@planetscale.com> test: fix end to end test expectations and add a new one Signed-off-by: Manan Gupta <manan@planetscale.com> feat: mark tablets as not reachable if the sql thread is stopped Signed-off-by: Manan Gupta <manan@planetscale.com> test: augment the end to end test to also check for just sql thread stopped Signed-off-by: Manan Gupta <manan@planetscale.com> feat: change sqlthreadwasrunning to first convert from proto to replication status Signed-off-by: Manan Gupta <manan@planetscale.com> test: fix stopReplicationAndGetStatusMaps test to provide gtid set which is correct Signed-off-by: Manan Gupta <manan@planetscale.com> * test: add missing defer teardown in initialization tests Signed-off-by: Manan Gupta <manan@planetscale.com> Co-authored-by: Matt Lord <mattalord@gmail.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.
Description
We had added the code that started the
SQL_ThreadinWaitSourcePosto handle the case where a tablet with replication stopped in ERS would have a chance to succeed as we then wait for it to apply its relay logs.But, mutating the state there in
WaitSourcePosresults in race conditions with tablet repairs that can leave replication broken and requiring manual repair using usingSTART SLAVE.So, we have decided to revert that mutation change and instead in ERS we treat tablets that have their
SQL_Threadstopped as though they were unreachable. If this prevents ERS from running a failover entirely — because of multiple failure detections — the user will need to either explicitly ignore the tablet or manually start replication on it. This guarantees safety since the responsibility of what to do in this scenario resides with the user, and this change will also play well withvtorcsince in this case it will start the replication on the tablet eventually and call ERS again.This reverts the state mutating parts from #9512 and the follow-up #10104 and fixes ERS as described above.
It leaves the other helper functions in place as they could be useful for other purposes at some point. They are effectively unused though so we could remove them as well since it's currently dead code.
Related Issue(s)
Checklist