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

syncer,pkg/schema: implement schema tracker #379

Merged
merged 12 commits into from
Jan 2, 2020
Merged

syncer,pkg/schema: implement schema tracker #379

merged 12 commits into from
Jan 2, 2020

Conversation

kennytm
Copy link
Contributor

@kennytm kennytm commented Nov 28, 2019 •
edited
Loading

What problem does this PR solve?

Stores a copy of the downstream schema in the syncer, and whenever a DDL is received from upstream, execute this on the copy to track the latest schema.

What is changed and how it works?

  • Use the TiDB library with Mock-TiKV to store the schema
  • Changed most types to use model.*

(TODO: save the schema into checkpoint)

Check List

Tests

  • Unit test

Code changes

  • Has persistent data change

Side effects

  • Possible performance regression
    • all "caches" are removed for now.
  • Increased code complexity

Related changes

  • Need to update the documentation
    • explain the relaxed requirement after using schema tracker.
  • Need to be included in the release note

@kennytm kennytm added priority/normal Minor change, requires approval from ≥1 primary reviewer needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated status/WIP This PR is still work in progress type/feature New feature needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Nov 28, 2019
@kennytm kennytm force-pushed the kennytm/schema-tracker branch 14 times, most recently from 8169a88 to 6b76eb3 Compare December 5, 2019 09:19
@kennytm
Copy link
Contributor Author

kennytm commented Dec 6, 2019

/run-all-tests tidb=release-3.0

1 similar comment
@csuzhangxc
Copy link
Member

/run-all-tests tidb=release-3.0

@kennytm
Copy link
Contributor Author

kennytm commented Dec 17, 2019

/run-all-tests tidb=release-3.0

@codecov
Copy link

codecov bot commented Dec 17, 2019 •
edited
Loading

Codecov Report

Merging #379 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master       #379   +/-   ##
===========================================
  Coverage   59.1193%   59.1193%           
===========================================
  Files           167        167           
  Lines         18894      18894           
===========================================
  Hits          11170      11170           
  Misses         6682       6682           
  Partials       1042       1042

@kennytm kennytm changed the title [WIP] syncer,pkg/schema: implement schema tracker syncer,pkg/schema: implement schema tracker Dec 17, 2019
@kennytm kennytm marked this pull request as ready for review December 17, 2019 10:37
@kennytm kennytm removed the status/WIP This PR is still work in progress label Dec 17, 2019
@kennytm
Copy link
Contributor Author

kennytm commented Dec 21, 2019

/run-all-tests tidb=release-3.0

csuzhangxc
csuzhangxc reviewed Dec 23, 2019
View reviewed changes
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

syncer/dml.go Show resolved Hide resolved
syncer/syncer.go Outdated Show resolved Hide resolved
syncer/syncer_test.go Show resolved Hide resolved
pkg/terror/error_list.go Show resolved Hide resolved
@csuzhangxc
Copy link
Member

@WangXiangUSTC PTAL

WangXiangUSTC
WangXiangUSTC reviewed Dec 24, 2019
View reviewed changes
pkg/schema/tracker.go Show resolved Hide resolved
pkg/schema/tracker.go Outdated Show resolved Hide resolved
pkg/schema/tracker.go Show resolved Hide resolved
syncer/syncer.go Outdated Show resolved Hide resolved
@kennytm
Copy link
Contributor Author

kennytm commented Dec 24, 2019

/run-all-tests tidb=release-3.0

csuzhangxc
csuzhangxc reviewed Dec 25, 2019
View reviewed changes
syncer/syncer.go Outdated Show resolved Hide resolved
pkg/terror/error_list.go Outdated Show resolved Hide resolved
@kennytm
Copy link
Contributor Author

kennytm commented Dec 25, 2019

/run-all-tests tidb=release-3.0

csuzhangxc
csuzhangxc reviewed Dec 27, 2019
View reviewed changes
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 Dec 27, 2019
@csuzhangxc
Copy link
Member

@WangXiangUSTC PTAL again

WangXiangUSTC
WangXiangUSTC reviewed Dec 31, 2019
View reviewed changes
pkg/schema/tracker.go Show resolved Hide resolved
syncer/checkpoint.go Show resolved Hide resolved
syncer/checkpoint.go Show resolved Hide resolved
@WangXiangUSTC
Copy link
Contributor

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Dec 31, 2019
@WangXiangUSTC
Copy link
Contributor

/run-all-tests tidb=release-3.0

1 similar comment
@kennytm
Copy link
Contributor Author

kennytm commented Dec 31, 2019

/run-all-tests tidb=release-3.0

@csuzhangxc csuzhangxc merged commit 4bdce00 into master Jan 2, 2020
@csuzhangxc csuzhangxc deleted the kennytm/schema-tracker branch January 2, 2020 08:20
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
@kennytm @csuzhangxc
syncer,pkg/schema: implement schema tracker (pingcap#379)
a090324
@csuzhangxc csuzhangxc removed the needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated label Jun 17, 2020
@csuzhangxc csuzhangxc added already-update-docs The docs related to this PR already updated. Add this label once the docs are updated and removed needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated labels Oct 27, 2020
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@csuzhangxc csuzhangxc csuzhangxc left review comments

@WangXiangUSTC WangXiangUSTC WangXiangUSTC approved these changes

Assignees
No one assigned
Labels
already-update-docs The docs related to this PR already updated. Add this label once the docs are updated priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/feature New feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kennytm @csuzhangxc @WangXiangUSTC