Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

Proposal: Binlog replication(syncer) unit support plugin #596

Merged
merged 11 commits into from
May 12, 2020

Conversation

WangXiangUSTC
Copy link
Contributor

What problem does this PR solve?

Add a proposal for supporting plugin in binlog replication(syncer) unit

@WangXiangUSTC WangXiangUSTC added status/PTAL This PR is ready for review. Add this label back after committing new changes type/doc Documentation changes in this repro labels Apr 12, 2020
@codecov
Copy link

codecov bot commented Apr 12, 2020

Codecov Report

Merging #596 into master will decrease coverage by 0.8697%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master       #596        +/-   ##
================================================
- Coverage   58.5644%   57.6947%   -0.8698%     
================================================
  Files           204        203         -1     
  Lines         21344      20579       -765     
================================================
- Hits          12500      11873       -627     
+ Misses         7671       7557       -114     
+ Partials       1173       1149        -24     

Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

rest LGTM

docs/RFCS/20200412_syncer_plugin.md Outdated Show resolved Hide resolved
docs/RFCS/20200412_syncer_plugin.md Show resolved Hide resolved
@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

rest LGTM

docs/RFCS/20200412_syncer_plugin.md Show resolved Hide resolved
Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels May 9, 2020
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

LGTM

docs/RFCS/20200412_syncer_plugin.md Outdated Show resolved Hide resolved

`HandleDMLJobResult` handle the result of [handleRowsEvent](https://github.com/pingcap/dm/blob/9023c789964fde0f5134e0c49435db557e21fdf7/syncer/syncer.go#L1274) and then do something.

NOTE: We use `runFatalChan` to report errors between goroutines for executing ddl and dml, and the `err` of `handleQueryEvent` and `handleRowsEvent` don't contain the real error messag. Need to refine it.
Copy link
Contributor

@lichunzhu lichunzhu May 9, 2020

Choose a reason for hiding this comment

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

The atomic error execError used in this #605 may fix this problem 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I delete this note in 4e6b864

Co-authored-by: Chunzhu Li <lichunzhu@stu.xjtu.edu.cn>
@lichunzhu lichunzhu added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels May 11, 2020
lichunzhu
lichunzhu previously approved these changes May 11, 2020
@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

1 similar comment
@WangXiangUSTC
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

LGTM

@lichunzhu lichunzhu merged commit 2428d59 into master May 12, 2020
@WangXiangUSTC WangXiangUSTC deleted the xiang/plugin branch May 12, 2020 08:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/LGT2 Two reviewers already commented LGTM, ready for merge type/doc Documentation changes in this repro
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants