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

ddl: rollingback add index meets panic leads json unmarshal object error #23848

Merged
merged 17 commits into from
May 6, 2021
Merged

ddl: rollingback add index meets panic leads json unmarshal object error #23848

merged 17 commits into from
May 6, 2021

Conversation

Howie59
Copy link
Contributor

@Howie59 Howie59 commented Apr 5, 2021

What problem does this PR solve?

Issue Number: close #23321

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Release note

  • No release note

@ti-chi-bot ti-chi-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 5, 2021
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Apr 5, 2021
@Howie59
change pos
ec2ce0a 
Signed-off-by: lihaowei <haoweili35@gmail.com>
zimulala
zimulala reviewed Apr 6, 2021
View reviewed changes
ddl/db_test.go Outdated Show resolved Hide resolved
zimulala
zimulala reviewed Apr 6, 2021
View reviewed changes
ddl/db_test.go Outdated Show resolved Hide resolved
@zimulala zimulala added the type/bugfix This PR fixes a bug. label Apr 6, 2021
@Howie59
annotation
5305f96 
Signed-off-by: lihaowei <haoweili35@gmail.com>
xiongjiwei
xiongjiwei reviewed Apr 6, 2021
View reviewed changes
ddl/db_test.go Outdated Show resolved Hide resolved
@Howie59
assert failpoint err
cb21482 
Signed-off-by: lihaowei <haoweili35@gmail.com>
@Howie59
Copy link
Contributor Author

Howie59 commented Apr 6, 2021

/cc @zimulala @xiongjiwei

@xhebox
Copy link
Contributor

xhebox commented Apr 6, 2021

I did not follow the issue, does the problem exist on the master? Should we file PR merged to the master?

@xiongjiwei
Copy link
Contributor

xiongjiwei commented Apr 7, 2021 •
edited
Loading

/cc @AilinKid PTAL. Do we decided how to fix the issue?

@AilinKid
Copy link
Contributor

AilinKid commented Apr 7, 2021

/cc @AilinKid PTAL. Do we decided how to fix the issue?

I think just to check it in here is adequate for now

@AilinKid
Copy link
Contributor

AilinKid commented Apr 7, 2021

actually, the master can also get the phenomenon above, but the error returned to the statement is substituted by another commit. So we can't see the JSON unmarshal error directly, but if we debug the internal step, we can see the intermediate error is also JSON unmarshal error.

so we can apply the same logic to the master @Howie59

@Howie59 Howie59 mentioned this pull request Apr 8, 2021
Merged
@Howie59
Copy link
Contributor Author

Howie59 commented Apr 8, 2021

actually, the master can also get the phenomenon above, but the error returned to the statement is substituted by another commit. So we can't see the JSON unmarshal error directly, but if we debug the internal step, we can see the intermediate error is also JSON unmarshal error.

so we can apply the same logic to the master @Howie59

i submit it in issue #23913

zimulala
zimulala reviewed Apr 13, 2021
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
Copy link
Contributor

/LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 13, 2021
@zimulala
Copy link
Contributor

PTAL @AilinKid

@zimulala
Copy link
Contributor

@Howie59
Could we merge #23913 first? Then we cherry-pick it to v4.0 and v5.0.

AilinKid
AilinKid reviewed Apr 13, 2021
View reviewed changes
Copy link
Contributor

@AilinKid AilinKid 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

ddl/ddl_worker.go Show resolved Hide resolved
@Howie59
add err msg
a7a3655 
Signed-off-by: lihaowei <haoweili35@gmail.com>
@Howie59
directly return
47e1ec0 
Signed-off-by: lihaowei <haoweili35@gmail.com>
@ti-chi-bot ti-chi-bot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 14, 2021
@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Apr 16, 2021
@wjhuang2016
Copy link
Member

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 60ec2fa

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 16, 2021
@zimulala
Copy link
Contributor

/run-integration-compatibility-test

@bb7133
Copy link
Member

bb7133 commented Apr 19, 2021

/merge

@ti-chi-bot
Copy link
Member

@Howie59: /merge is only allowed for the committers in list.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@xhebox
Copy link
Contributor

xhebox commented Apr 25, 2021

/run-tics-test

@Howie59
Copy link
Contributor Author

Howie59 commented Apr 25, 2021

/run-unit-test

1 similar comment
@Howie59
Copy link
Contributor Author

Howie59 commented Apr 30, 2021

/run-unit-test

@zhouqiang-cl zhouqiang-cl added the cherry-pick-approved Cherry pick PR approved by release team. label Apr 30, 2021
@zhouqiang-cl
Copy link
Contributor

/merge

@zhouqiang-cl
Copy link
Contributor

/run-unit-tests

@zhouqiang-cl
Copy link
Contributor

/merge

@xiongjiwei
Copy link
Contributor

/merge

@Howie59
Copy link
Contributor Author

Howie59 commented May 6, 2021

/run-common-test

@ti-chi-bot ti-chi-bot merged commit 71b0df9 into pingcap:release-4.0 May 6, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request May 6, 2021
@Howie59 @ti-srebot
cherry pick pingcap#23848 to release-5.0
a915500 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request May 6, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #24441

ti-chi-bot pushed a commit that referenced this pull request Aug 13, 2021
@ti-srebot
ddl: rollingback add index meets panic leads json unmarshal object er…
d969bcb 
…ror (#23848) (#24441)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AilinKid AilinKid AilinKid left review comments

@zimulala zimulala zimulala left review comments

@xiongjiwei xiongjiwei Awaiting requested review from xiongjiwei

Assignees
No one assigned
Labels
cherry-pick-approved Cherry pick PR approved by release team. needs-cherry-pick-release-5.0 sig/sql-infra SIG: SQL Infra size/S Denotes a PR that changes 10-29 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@Howie59 @xhebox @xiongjiwei @AilinKid @zimulala @ti-chi-bot @wjhuang2016 @bb7133 @zhouqiang-cl @ti-srebot