Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

Support table routing rules (merging sharded tables) #95

Merged
merged 10 commits into from
May 4, 2019

Conversation

kennytm
Copy link
Collaborator

@kennytm kennytm commented Dec 7, 2018

What problem does this PR solve?

TOOL-142

(Note: still won't handle UNIQUE/PRIMARY key conflict. This needs column-mapping)

What is changed and how it works?

Rename the tables while the loading the files. Since we don't care about the table name when parsing the data files, it becomes very simple to support merging: just associate all those data files to the target table.

This PR supersedes #54. Note that #54 is very large because it attempts to do some refactoring at the same time.

Check List

Tests

  • Integration test

Code changes

Side effects

Related changes

  • Need to update the documentation
  • Need to be included in the release note

@kennytm kennytm added status/DNM Do not merge, test is failing or blocked by another PR Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated labels Dec 7, 2018
@sre-bot
Copy link

sre-bot commented Dec 7, 2018

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.

@kennytm
Copy link
Collaborator Author

kennytm commented Dec 7, 2018

/run-all-tests

@kennytm kennytm force-pushed the kennytm/shard-merging branch 2 times, most recently from e736496 to 5b1aaf1 Compare December 10, 2018 16:30
@kennytm
Copy link
Collaborator Author

kennytm commented Dec 13, 2018

/run-all-tests

@kennytm kennytm removed the status/DNM Do not merge, test is failing or blocked by another PR label Dec 13, 2018
@kennytm kennytm changed the title [DNM] Support table routing rules (merging sharded tables) Support table routing rules (merging sharded tables) Dec 14, 2018
@kennytm
Copy link
Collaborator Author

kennytm commented Dec 14, 2018

/run-integration-test

@kennytm kennytm added the status/PTAL This PR is ready for review. Add this label back after committing new changes label Dec 14, 2018
@kennytm
Copy link
Collaborator Author

kennytm commented Dec 16, 2018

PTAL @july2993 @csuzhangxc

@kennytm
Copy link
Collaborator Author

kennytm commented Feb 16, 2019

PTAL @lonng @july2993

@kennytm kennytm added status/WIP Work in progress status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/PTAL This PR is ready for review. Add this label back after committing new changes status/WIP Work in progress labels Mar 20, 2019
@kennytm
Copy link
Collaborator Author

kennytm commented Apr 29, 2019

/run-all-tests tidb=master tikv=master pd=master

@IANTHEREAL
Copy link
Collaborator

LGTM

@IANTHEREAL
Copy link
Collaborator

@csuzhangxc @lonng PTAL

@IANTHEREAL IANTHEREAL added status/LGT1 One reviewer already commented LGTM (LGTM1) and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Apr 30, 2019
@csuzhangxc
Copy link
Member

Is there any problem with CI?

@kennytm
Copy link
Collaborator Author

kennytm commented May 1, 2019

There's a bug in Restore where the type DECIMAL(20,0) is restored as DECIMAL (which means DECIMAL(11,0)), causing error since 99999999999999999999 cannot be represented in the latter type.

Edit: filed pingcap/parser#310

@kennytm
Copy link
Collaborator Author

kennytm commented May 1, 2019

/run-all-tests tidb=master tikv=master pd=master

@kennytm

This comment has been minimized.

@kennytm
Copy link
Collaborator Author

kennytm commented May 1, 2019

/run-all-tests tidb=release-3.0 tikv=release-3.0 pd=release-3.0

@kennytm
Copy link
Collaborator Author

kennytm commented May 1, 2019

/run-all-tests tidb=release-2.1 tikv=release-2.1 pd=release-2.1

@kennytm
Copy link
Collaborator Author

kennytm commented May 1, 2019

I think we've got a TiDB regression from 2.1 to 3.0.

Edit: Reproduced locally, it's a master→master regression.

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/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels May 4, 2019
@kennytm kennytm merged commit 748f7e2 into master May 4, 2019
@kennytm kennytm deleted the kennytm/shard-merging branch May 4, 2019 11:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Should Update Docs Should update docs after this PR is merged. Remove this label once the docs are updated status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) type/feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants