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: speedup test case TestModifyColumnTime #24469

Merged
merged 9 commits into from
May 11, 2021
Merged

ddl: speedup test case TestModifyColumnTime #24469

merged 9 commits into from
May 11, 2021

Conversation

jianzhiyao
Copy link
Contributor

@jianzhiyao jianzhiyao commented May 7, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: #24459

Problem Summary:

What is changed and how it works?

split testDBSuite1.TestModifyColumnTime into many parts

build & test time

Comparing with pr: #24460, test time is reduced about 50s

pr check_dev check_dev_2
#24460 3m 7s(link) 6m 10s(link)
#24469 2m 13s(link) 5m 15s(link)

Check List

Tests

  • No code

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM

Release note

  • ddl: speedup test case TestModifyColumnTime

@ti-chi-bot ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 7, 2021
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label May 7, 2021
@jianzhiyao jianzhiyao force-pushed the frt/speedup_test_modify_time branch from f9fa804 to e20c345 Compare May 7, 2021 12:41
zimulala
zimulala reviewed May 8, 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.

Could you add a description of how long the tests were executed before and after this PR?

ddl/db_test.go Outdated
c.Assert(err, IsNil, Commentf("%+v", t))
var wg sync.WaitGroup
wg.Add(len(tests))
for i, t := range tests {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary to drive so many goroutines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so :-)

@jianzhiyao jianzhiyao force-pushed the frt/speedup_test_modify_time branch 4 times, most recently from d7f654b to 8b9c2fe Compare May 8, 2021 04:05
@jianzhiyao jianzhiyao requested a review from zimulala May 8, 2021 10:00
@xhebox
Copy link
Contributor

xhebox commented May 10, 2021 •
edited
Loading

@jianzhiyao Could you use the same trick as in #24487 ? Testcases in this test are actually standalone. I think splitted tests are better than shooting goroutines. Of course, only if it is indeed faster.

@jianzhiyao jianzhiyao force-pushed the frt/speedup_test_modify_time branch from 8b9c2fe to a5e7098 Compare May 10, 2021 06:21
@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 10, 2021
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 10, 2021
@pingcap pingcap deleted a comment from ti-chi-bot May 10, 2021
@AilinKid AilinKid removed the status/LGT1 Indicates that a PR has LGTM 1. label May 10, 2021
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 10, 2021
@AilinKid
Copy link
Contributor

/dissmiss

xhebox
xhebox reviewed May 10, 2021
View reviewed changes
Copy link
Contributor

@xhebox xhebox 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/db_test.go
variable.SetDDLErrorCountLimit(limit)
tk.Se.GetSessionVars().EnableChangeColumnType = enableChangeColumnType
tk.Se.GetSessionVars().TimeZone = originalTz
}()

for _, t := range tests {
tk.MustExec("drop table if exists t_mc")
Copy link
Contributor

@xhebox xhebox May 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Tablename duplicated for cases, plz fix.

Copy link
Contributor Author

@jianzhiyao jianzhiyao May 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

According to the result of check_dev and check_dev_2 , seems tablename can be the same in different test case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Testcases could be paralleled, under which case it will fail... But I'll give you LGTM anyway.

@jianzhiyao
Copy link
Contributor Author

/dissmiss

PTAL again after test passed

@jianzhiyao jianzhiyao requested a review from xhebox May 10, 2021 07:06
@xhebox
Copy link
Contributor

xhebox commented May 10, 2021

/lgtm

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • xhebox

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 10, 2021
@xhebox
Copy link
Contributor

xhebox commented May 10, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 1b1975c

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 10, 2021
@ti-chi-bot
Copy link
Member

/run-all-tests

This bot automatically retries jobs that failed on can merge PRs (send feedback to hi-rustin).

Silence the bot with the /merge cancel comment for consistent failures.

1 similar comment
@ti-chi-bot
Copy link
Member

/run-all-tests

This bot automatically retries jobs that failed on can merge PRs (send feedback to hi-rustin).

Silence the bot with the /merge cancel comment for consistent failures.

@jianzhiyao
Copy link
Contributor Author

/run-common-test

@jianzhiyao
Copy link
Contributor Author

/run-integration-common-test

@jianzhiyao
Copy link
Contributor Author

/run-common-test

@jianzhiyao
Copy link
Contributor Author

/run-unit-test

@Rustin170506
Copy link
Member

/run-all-tests

@Rustin170506
Copy link
Member

/run-integration-ddl-test

@jianzhiyao
Copy link
Contributor Author

/run-tics-test
/run-sqllogic-test-1
/run-sqllogic-test-2
/run-integration-ddl-test

@ti-chi-bot
Copy link
Member

@jianzhiyao: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

@ti-chi-bot ti-chi-bot merged commit 24e9c13 into pingcap:master May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimulala zimulala Awaiting requested review from zimulala

@xhebox xhebox Awaiting requested review from xhebox

Assignees
No one assigned
Labels
component/test sig/sql-infra SIG: SQL Infra size/L Denotes a PR that changes 100-499 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jianzhiyao @xhebox @AilinKid @ti-chi-bot @Rustin170506 @zimulala