-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Backwards compatible replication status to state transition #10167
Conversation
It was NOT backwards compatible. Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
We are only appending the last io_thread_connecting field Signed-off-by: Matt Lord <mattalord@gmail.com>
1c5dc7a
to
0f8d71b
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
0f8d71b
to
3dd7895
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
669916a
to
dd34bdc
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
965ca02
to
3defa0f
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
3defa0f
to
e340dfc
Compare
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
… compatible Signed-off-by: Manan Gupta <manan@planetscale.com>
… the upgrade Signed-off-by: Manan Gupta <manan@planetscale.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this all Looks good now! Maybe it is better we get a review from @deepthi too before merging
Signed-off-by: Manan Gupta <manan@planetscale.com>
LGTM! Thanks for all the help, @GuptaManan100 ! ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
Apologies for missing the compatibility issue on the earlier PR.
@@ -91,6 +91,21 @@ func ReplicationStatusToProto(s ReplicationStatus) *replicationdatapb.Status { | |||
SqlState: int32(s.SQLState), | |||
LastSqlError: s.LastSQLError, | |||
} | |||
|
|||
// We need to be able to send gRPC response messages from v14 and newer tablets to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Description
It turns out that this was NOT fully backwards compatible: https://github.com/vitessio/vitess/pull/9853/files
Later ERS related upgrade/downgrade tests added in #10148 showed this. The problem was that a v14
vtctl
(client:vtctlclient
->vtctld
orvtctl
) would send an RPC to a v13vttablet
(server) and thevttablet
would respond with the v13ReplicationStatusResponse
message that does not have theIoStatus
orSqlStatus
fields in it so the v14vtctl
would process that response and consider the IO and SQL statuses as Unknown and be unable to proceed with some actions (such as reparenting). In order to support a v14vtctl
with v13vttablet
s, when processing the RPC response we need to check for an IO and SQL state of Unknown and revert to using the binary IO and SQL thread running values (the older tablets cannot tell us if the IO state was connecting).In order to support correct RPC communication between v13 and v14 we must also leave the old fields in place so that it's a clean upgrade from v13 to v14 — and the v14
vtctl
will process the olderio_thread_running
andsql_thread_running
ReplicationStatusResponse
fields — while adding the new fields for v14+vttablets
and clients to use. This way in v15+ we can remove the old fields and complete the transition for smooth upgrades.Related Issue(s)
Checklist