Skip to content
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

go/mysql: Fix MariadbGTIDSet multi-domain support. #6184

Merged
merged 1 commit into from
May 15, 2020

Conversation

enisoc
Copy link
Member

@enisoc enisoc commented May 15, 2020

This fixes two bugs:

  1. AddGTID was mutating the original set. GTIDSet implementations are
    supposed to be immutable.

  2. AddGTID and parseMariadbGTIDSet did not enforce any order among
    domains, yet Equal and other functions expected order to be enforced.

This switches from a list to a map to better represent the fact that the order of domains doesn't matter, which also simplifies some code. We always enforce order when serializing back to a string.

@enisoc enisoc requested a review from deepthi May 15, 2020 02:15
@enisoc enisoc requested a review from sougou as a code owner May 15, 2020 02:15
@enisoc enisoc force-pushed the mariadb-gtid-set branch from 01b8f03 to 9a706fb Compare May 15, 2020 15:40
This fixes two bugs:

1. AddGTID was mutating the original set. GTIDSet implementations are
supposed to be immutable.

2. AddGTID and parseMariadbGTIDSet did not enforce any order among
domains, yet Equal and other functions expected order to be enforced.

This switches from a list to a map to better represent the fact that the
order of domains doesn't matter, which also simplifies some code.
We always enforce order when serializing back to a string.

Signed-off-by: Anthony Yeh <enisoc@planetscale.com>
@enisoc enisoc force-pushed the mariadb-gtid-set branch from 9a706fb to aa865ea Compare May 15, 2020 16:37
@enisoc
Copy link
Member Author

enisoc commented May 15, 2020

@deepthi I got the tests fixed so this is ready for review now.

@PrismaPhonic I can't add you as a reviewer, but I'd appreciate if you can take a look.

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the Domain monotonically increasing in the Position string we get from MariaDB? If we parse that string into GTID and convert back to string, do we get back the same value?

@enisoc
Copy link
Member Author

enisoc commented May 15, 2020

Is the Domain monotonically increasing in the Position string we get from MariaDB?

It looks that way in the examples I've seen in the docs, but I haven't found any documented guarantee that it's always the case.

If we parse that string into GTID and convert back to string, do we get back the same value?

Given the above, we can't be sure, short of checking the MariaDB code. Are you thinking of a case in which it might be a problem to send back a sorted value when given an out-of-order value?

@deepthi
Copy link
Member

deepthi commented May 15, 2020

Given the above, we can't be sure, short of checking the MariaDB code. Are you thinking of a case in which it might be a problem to send back a sorted value when given an out-of-order value?

Right. Anywhere where we do replicate until semantics and pass in a stop position.
Let us assume MariaDB hands them out sorted and we preserve the sorting. We are still better off than the previous implemetation.

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@deepthi deepthi merged commit eb21175 into vitessio:master May 15, 2020
@enisoc enisoc deleted the mariadb-gtid-set branch May 15, 2020 19:13
@enisoc
Copy link
Member Author

enisoc commented May 15, 2020

Anywhere where we do replicate until semantics and pass in a stop position.

Do you think it would be a problem to hand back the domains in a different order? I would assume MariaDB internally parses and/or compares them in such a way that order doesn't matter, though I haven't found documented proof of this.

@deepthi
Copy link
Member

deepthi commented May 15, 2020

Do you think it would be a problem to hand back the domains in a different order? I would assume MariaDB internally parses and/or compares them in such a way that order doesn't matter, though I haven't found documented proof of this.

I don't know. We could test it by running a vitess cluster on mariadb.

@deepthi deepthi added this to the v7.0 milestone Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants