-
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
VDiff: Support a max diff time for tables #14786
Conversation
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Matt Lord <[email protected]>
aa66f26
to
67d55a9
Compare
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
2967b12
to
78b7915
Compare
Signed-off-by: Matt Lord <[email protected]>
78b7915
to
e75bad7
Compare
Signed-off-by: Matt Lord <[email protected]>
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! just a few questions that shouldn't affect mergability
@@ -267,6 +318,7 @@ func testCLIFlagHandling(t *testing.T, targetKs, workflowName string, cell *Cell | |||
AutoRetry: true, | |||
UpdateTableStats: true, | |||
TimeoutSeconds: 60, | |||
MaxDiffSeconds: 333, |
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.
333 seems like an oddly-specific duration to me ... is there a reason you went with this?
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.
Only in that I needed to pick something specific and this is the random number I typed. 🙂
@@ -52,6 +52,9 @@ import ( | |||
// how long to wait for background operations to complete | |||
var BackgroundOperationTimeout = topo.RemoteOperationTimeout * 4 | |||
|
|||
var ErrMaxDiffDurationExceeded = vterrors.Errorf(vtrpcpb.Code_DEADLINE_EXCEEDED, "table diff was stopped due to exceeding the max-diff-duration time") |
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.
nice
@@ -621,6 +621,7 @@ message VDiffCoreOptions { | |||
int64 timeout_seconds = 6; | |||
int64 max_extra_rows_to_compare = 7; | |||
bool update_table_stats = 8; | |||
int64 max_diff_seconds = 9; |
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.
this is more a general question, since the other duration-type in this message is also int64
: is there a reason we do the "int as seconds" thing here rather than using vttime.Duration
like we do in vtctldata and others?
(also extremely nit-picky and GH won't let me comment on the line, but i'm spotting a double-space:
- int64 max_rows = 3;
+ int64 max_rows = 3;
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 thought about that too, but vttime is a struct with a seconds and nanoseconds value and in this case I only cared about the seconds so just used that part of it. I'm fine changing it back though too.
I'll fix the spacing. Thanks!
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'm good with the consistency! (i.e. don't change it)
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
Signed-off-by: Matt Lord <[email protected]>
We merged these two PRs concurrently and thus ended up with a breakage: - vitessio#14735 - vitessio#14786 This PR addresses that issue by using a locally scoped vtgate connection according to the newly established model. Signed-off-by: Matt Lord <[email protected]>
Description
This PR adds a VDiff create flag called
--max-diff-duration
which allows a user to limit the maximum runtime for a single table diff run (the table diff can span N runs). This allows a user to further limit the impact of a VDiff on their production systems. Please see the issue for more information on the use case.Click here to see a simple manual test:
Final results being:
Related Issue(s)
Checklist