refactor drainer syncer #532
Conversation
|
/run-unit-test |
|
Hi contributor, thanks for your PR. This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically. |
|
/run-unit-test |
|
/run-all-tests |
|
/run-unit-test |
|
/run-unit-test |
|
It took me more than 75 minutes to review this PR. 😿 |
much quicker than as expected by me |
|
/run-all-tests |
|
PTAL @suzaku @GregoryIan |
|
/run-all-tests |
| func (p *KafkaSyncer) Close() error { | ||
| close(p.shutdown) | ||
|
|
||
| err := <-p.Error() |
There was a problem hiding this comment.
If no error occurs, will this line block forever?
There was a problem hiding this comment.
may return if SetErr is called, and may return nil
There was a problem hiding this comment.
func (t *testExecutorSuite) TestBaseError(c *C) {
be := newBaseError()
select {
case err := <-be.Error():
c.Assert(err, IsNil)
case <-time.After(time.Second):
c.Fatal("Timeout")
}
}If SetErr is not called, it may wait forever.
There was a problem hiding this comment.
yes, as what i said.
There was a problem hiding this comment.
If we can not be sure there must be something wrong when close is called, maybe we should use select to avoid blocking?
|
LGTM |
|
/run-all-tests |
|
| if !ok { | ||
| return errors.Errorf("not found table id: %d", mutation.GetTableId()) | ||
| return false, errors.Errorf("not found table id: %d", mutation.GetTableId()) |
There was a problem hiding this comment.
how about use errors.NotFoundf
There was a problem hiding this comment.
pingcap/errors#11
we should not use errors.NotFoundf, instead we can remove it all, keep this now.
|
/run-all-tests |
|
LGTM |
Drop the Check() and and String() of Checkpoint. Drop MetaCheckpoint of FlashCheckPoint, after pingcap#532 there's no need to keep this.
What problem does this PR solve?
https://internal.pingcap.net/jira/browse/TOOL-963
discard the origin Translator and executor interface, add a new
Syncerinterface indrainer/sync/syncer.go/sync/translatorcovered more than 75%, making total coverage from 45.625% to 53.721%bad side:
What is changed and how it works?
discard the origin Translator and executor interface, add a new Syncer interface
Check List
Tests
Code changes
Side effects
Related changes