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

vreplication: file:pos flavor #5432

Merged
merged 7 commits into from
Nov 23, 2019
Merged

Conversation

sougou
Copy link
Contributor

@sougou sougou commented Nov 13, 2019

Fixes #5424

Details of the implementation are as described in the issue.
The tests are a little light right now. We'll probably need to add more as the flavor evolves.

Add Flavor as a conn param.
Add code to handle flavor-specific GTID.

Signed-off-by: Sugu Sougoumarane <[email protected]>
In this scheme, the filePos reader detects whether we are in a
transaction or not, and emits appropriate GTID events.

Signed-off-by: Sugu Sougoumarane <[email protected]>
The vstreamer sent GTIDs "as they came". With the new change,
GTIDs are sent only when they matter: on COMMIT, DDL or OTHER.

This new approach makes the protocol easier to understand. Also,
it makes it easier for filePos to continuously send file and position.
The correct values will get used when significant events like
COMMIT are encountered.

Signed-off-by: Sugu Sougoumarane <[email protected]>
The vplayer currently uses ambiguous rules about how it handles the case
where a stop position was exceeded. As part of this change, we'll
standardize on: A stop position is considered to be successfully reached
if the new position is greater than or equal to the specified position.
The main motivation for this change is that the possibility of
position mismatch is higher in the case of file:pos tracking. We're
likely to hit many false positives if we're too strict.

Signed-off-by: Sugu Sougoumarane <[email protected]>
Signed-off-by: Sugu Sougoumarane <[email protected]>
@sougou sougou requested review from rafael and deepthi November 13, 2019 06:28
Signed-off-by: Sugu Sougoumarane <[email protected]>
Copy link
Member

@rafael rafael left a comment

Choose a reason for hiding this comment

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

@sougou - This makes sense to me and the implementation is more straight forward than I was thinking :D

I think we can add more tests later.

The only part I'm a bit hesitant is the changes in the logic to stop position. If you could elaborate a bit more on why that is safe or why it won't introduce any regressions, I think we should merge this.

I'm going to approve but let you merge after talking about the stop position bit.

@rafael
Copy link
Member

rafael commented Nov 23, 2019

@sougou and I discussed about the stop changes in behavior. The main gist is:

It normally cannot happen but if it does, the system fails in extremely bad ways. For example, it will just keep going forward without stopping. With the new code, it will at least stop.

I'm going to merge this and integrate it in #5289

@rafael rafael merged commit 848f82b into vitessio:master Nov 23, 2019
@sougou sougou deleted the ss-filepos branch November 24, 2019 02:04
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.

RFC: File:Position based VReplication
2 participants