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.

Sign up for GitHub

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

Jump to bottom

*: clean code for restore table #9090

Merged
merged 13 commits into from
Apr 8, 2019
Merged

*: clean code for restore table #9090

merged 13 commits into from
Apr 8, 2019

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Jan 16, 2019 •
edited
Loading

What problem does this PR solve?

change

admin restore table [table_name]

syntax to

recover table [table_name]

syntax.
clean code for restore table.
related PR: #7937
related parser PR: pingcap/parser#180

What is changed and how it works?

move restore table from adminStmt to ddl stmt. Because recover table is a ddl operation.
remove redundancy code.

Check List

Tests

  • No test

Code changes

  • Has exported function/method change

Side effects

Related changes

@crazycs520 crazycs520 requested a review from zimulala January 16, 2019 08:49
@crazycs520 crazycs520 mentioned this pull request Jan 16, 2019
Merged
@winkyao
Copy link
Contributor

winkyao commented Jan 16, 2019

Please fix ci

@crazycs520
Copy link
Contributor Author

Ci failed because of the parser go.mod, after merge parser PR, I will fix this. PTAL first.

@crazycs520
Copy link
Contributor Author

/run-all-tests

zimulala
zimulala reviewed Jan 23, 2019
View reviewed changes
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 23, 2019
@tiancaiamao
Copy link
Contributor

Please fix ci @crazycs520

@crazycs520
Copy link
Contributor Author

/run-all-tests

@tiancaiamao
Copy link
Contributor

LGTM
Is there a document about this statement and should we also update it?

@tiancaiamao tiancaiamao added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 1, 2019
@crazycs520
Copy link
Contributor Author

The proposal of this is use the ADMIN RESTORE TABLE table_id syntax, should we update the proposal? @winkyao

@winkyao
Copy link
Contributor

winkyao commented Apr 1, 2019 •
edited
Loading

@crazycs520 I don't think it is necessary, but you can add a paragraph to explain why the implement is not the same with the proposal.

winkyao
winkyao reviewed Apr 1, 2019
View reviewed changes
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@winkyao
Copy link
Contributor

winkyao commented Apr 1, 2019

Please resolve the conflicts

@codecov
Copy link

codecov bot commented Apr 8, 2019 •
edited
Loading

Codecov Report

Merging #9090 into master will decrease coverage by 0.055%.
The diff coverage is 91.8918%.

@@               Coverage Diff                @@
##             master      #9090        +/-   ##
================================================
- Coverage   78.0088%   77.9538%   -0.0551%     
================================================
  Files           404        404                
  Lines         82024      81996        -28     
================================================
- Hits          63986      63919        -67     
- Misses        13332      13361        +29     
- Partials       4706       4716        +10

@crazycs520
Copy link
Contributor Author

/run-all-tests

1 similar comment
@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520 crazycs520 removed the status/LGT2 Indicates that a PR has LGTM 2. label Apr 8, 2019
@crazycs520 crazycs520 added the status/LGT3 The PR has already had 3 LGTM. label Apr 8, 2019
@crazycs520 crazycs520 merged commit 8c118ce into pingcap:master Apr 8, 2019
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimulala zimulala zimulala left review comments

@winkyao winkyao winkyao approved these changes

Assignees
No one assigned
Labels
sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@crazycs520 @winkyao @tiancaiamao @zimulala @you06