Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

infoschema: Migrate the infoschema's retrieving data logic for 'CharacterSets/Collations' to executor #15103

Merged
merged 3 commits into from
Mar 3, 2020

Conversation

AndrewDi
Copy link
Contributor

@AndrewDi AndrewDi commented Mar 3, 2020

UCP #15040 #15035

What problem does this PR solve?

Infoschema contains too much logic for retrieving data, which is confusing, we need to migrate this part to the executor so that infoshema only contains schema information. Detail ref #15040 #15035

What is changed and how it works?

migrate function dataForCharacterSets/dataForCollations from infoschema pkg to executor/infoschema_reader.go

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Mar 3, 2020
@AndrewDi
Copy link
Contributor Author

AndrewDi commented Mar 3, 2020

@lonng @crazycs520 PTAL

@AndrewDi AndrewDi changed the title migrate dataForCharacterSets/dataForCollations from tables to executer infoschema: Migrate the infoschema's retrieving data logic for 'CharacterSets/Collations' to executor Mar 3, 2020
@lonng
Copy link
Contributor

lonng commented Mar 3, 2020

/rebuild

@codecov
Copy link

codecov bot commented Mar 3, 2020

Codecov Report

Merging #15103 into master will decrease coverage by 0.0401%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #15103        +/-   ##
================================================
- Coverage   80.3701%   80.3299%   -0.0402%     
================================================
  Files           502        502                
  Lines        131896     131896                
================================================
- Hits         106005     105952        -53     
- Misses        17527      17565        +38     
- Partials       8364       8379        +15

@AndrewDi
Copy link
Contributor Author

AndrewDi commented Mar 3, 2020

/run-all-tests

Copy link
Contributor

@lonng lonng left a comment

Choose a reason for hiding this comment

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

LGTM

@lonng lonng added sig/execution SIG execution component/infoschema status/LGT1 Indicates that a PR has LGTM 1. labels Mar 3, 2020
@reafans
Copy link
Contributor

reafans commented Mar 3, 2020

lgtm

@reafans reafans added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 3, 2020
@reafans
Copy link
Contributor

reafans commented Mar 3, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 3, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2020

@AndrewDi merge failed.

@lonng
Copy link
Contributor

lonng commented Mar 3, 2020

/rebuild

@lonng
Copy link
Contributor

lonng commented Mar 4, 2020

/award-point 100

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/infoschema contribution This PR is from a community contributor. sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants