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

ddl: fix panic in TestParallelDDL #16969

Merged
merged 8 commits into from
May 9, 2020
Merged

ddl: fix panic in TestParallelDDL #16969

merged 8 commits into from
May 9, 2020

Conversation

lance6716
Copy link
Contributor

@lance6716 lance6716 commented May 3, 2020

What problem does this PR solve?

Issue Number: close #15597

Problem Summary:

  1. MockContext failed interface type assertion, causing a panic. Proof see ddl_worker_test.go:testDDLSuite.TestParallelDDL failed #15597 (comment)

  2. try to reduce alloc slightly, BTW

What is changed and how it works?

  1. check type assertion and return statistics.PseudoRowCount on failure

  2. change 'make new slice, append old elements, append new element' to 'append new element to old slice'

Related changes

Check List

Tests

  • Unit test
  • Integration test

Side effects

Release note

  • No Release note

@sre-bot sre-bot added the contribution This PR is from a community contributor. label May 3, 2020
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label May 3, 2020
@codecov
Copy link

codecov bot commented May 3, 2020

Codecov Report

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

@@             Coverage Diff             @@
##             master     #16969   +/-   ##
===========================================
  Coverage   80.0143%   80.0143%           
===========================================
  Files           510        510           
  Lines        139245     139245           
===========================================
  Hits         111416     111416           
  Misses        18837      18837           
  Partials       8992       8992           

@lance6716
Copy link
Contributor Author

PTAL @SunRunAway @wjhuang2016

@lance6716
Copy link
Contributor Author

/run-all-tests

@lance6716
Copy link
Contributor Author

Almost one-line PR, PTAL @SunRunAway @wjhuang2016

ddl/reorg.go Outdated Show resolved Hide resolved
ddl/reorg.go Outdated Show resolved Hide resolved
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 status/LGT1 Indicates that a PR has LGTM 1. component/test labels May 9, 2020
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM

@AilinKid
Copy link
Contributor

AilinKid commented May 9, 2020

/run-all-tests

@bb7133 bb7133 added require-LGT3 Indicates that the PR requires three LGTM. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 9, 2020
@lance6716
Copy link
Contributor Author

@bb7133 Hi it's LGT3 in fact

@zimulala zimulala added status/LGT3 The PR has already had 3 LGTM. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT2 Indicates that a PR has LGTM 2. labels May 9, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 9, 2020

/run-all-tests

@zimulala
Copy link
Contributor

zimulala commented May 9, 2020

@lance6716
After CI passes, sre-bot will merge it. Thank you for your PR.

@sre-bot
Copy link
Contributor

sre-bot commented May 9, 2020

@lance6716 merge failed.

@zimulala
Copy link
Contributor

zimulala commented May 9, 2020

/run-integration-copr-test

Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133
Copy link
Member

bb7133 commented May 9, 2020

/rebuild

@zhouqiang-cl
Copy link
Contributor

/rebuild

1 similar comment
@zhouqiang-cl
Copy link
Contributor

/rebuild

@lance6716
Copy link
Contributor Author

/merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/test contribution This PR is from a community contributor. require-LGT3 Indicates that the PR requires three LGTM. sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ddl_worker_test.go:testDDLSuite.TestParallelDDL failed
8 participants