Adds new state to know that a diff has failed early#4209
Adds new state to know that a diff has failed early#4209sougou merged 1 commit intovitessio:masterfrom
Conversation
|
/cc @tirsen |
go/vt/worker/vertical_split_diff.go
Outdated
There was a problem hiding this comment.
nit: I suggest to rename "recorder" to "er" to be consistent with the rest of the code base.
systay
force-pushed
the
diff-failed-state
branch
from
63a8e05 to
59bbad2
Compare
|
I added a commit to respond to the review comments. Do you want me to squash it up into a single commit? |
|
@michael-berlin: Looks like comments are addressed. |
michael-berlin
left a comment
michael-berlin
left a comment
There was a problem hiding this comment.
General change LGTM. Just a couple minor comments.
go/vt/worker/split_diff.go
Outdated
There was a problem hiding this comment.
This is redundant and should be removed.
go/vt/worker/split_diff.go
Outdated
There was a problem hiding this comment.
Here you're not handling the "err == nil" case: If err == nil, then rec.RecordError() returned early and nothing happened.
There are similar comments below.
In practice what probably happens here is:
- your new code runs and sets the state to "diff is about to fail"
- diff actually finishes all the way to the end and then state is set to success
Ideally, the state transition code shouldn't allow that in the first place. But so far it's just doing simple overrides and no fancy checks.
To keep things simple, I suggest the following: Call your new method here only if err != nil.
go/vt/worker/split_diff.go
Outdated
There was a problem hiding this comment.
Same comment as above: Please only call here if err != nil.
go/vt/worker/vertical_split_diff.go
Outdated
There was a problem hiding this comment.
Skip err == nil case. Same comment as in the other file.
go/vt/worker/vertical_split_diff.go
Outdated
There was a problem hiding this comment.
Skip err == nil case. Same comment as in the other file.
go/vt/worker/split_diff.go
Outdated
There was a problem hiding this comment.
Technically, this doesn't mark the worker as failed.
Do you mind renaming it to "markAsWillFail"? That's closer to what's happening?
go/vt/worker/vertical_split_diff.go
Outdated
There was a problem hiding this comment.
Same comment as in the other file: Can you please rename to "markAsWillFail"?
|
Hi again @sougou and @michael-berlin. Good comments, thanks for catching that. Sorry for not fixing the comments sooner. Should I squash it up into a single commit? |
|
bump @sougou @michael-berlin |
|
This looks good. Can you resolve the conflicts? |
systay
force-pushed
the
diff-failed-state
branch
from
c83c77a to
5b08f0d
Compare
|
Rebased and squashed. |
go/apa.go
Outdated
There was a problem hiding this comment.
Looks like you caught this stray file.
Signed-off-by: Andres Taylor <antaylor@squareup.com>
systay
force-pushed
the
diff-failed-state
branch
from
5b08f0d to
b9abb67
Compare
3 participants
This makes it easy to see on the worker UI if a diff has failed without having to look through the log, even before it has finished running.