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

server: make CI faster #26546

Merged
merged 5 commits into from
Jul 27, 2021
Merged

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Problem Summary:

make failpoint-enable
cd server
go test

CI of the server package take too long...

What is changed and how it works?

Before:

ok      github.com/pingcap/tidb/server  120.812s

After:

ok      github.com/pingcap/tidb/server  49.372s

What's Changed:

Make some test case faster.

How it Works:

PASS: http_handler_test.go:1167: HTTPHandlerTestSuite.TestAllHistory	5.317s
PASS: http_handler_test.go:379: HTTPHandlerTestSuite.TestBinlogRecover	2.707s
PASS: http_handler_test.go:1401: HTTPHandlerTestSuite.TestDebugZip	1.244s
PASS: http_handler_test.go:870: HTTPHandlerTestSuite.TestDecodeColumnValue	5.324s
PASS: http_handler_test.go:934: HTTPHandlerTestSuite.TestGetIndexMVCC	5.290s
PASS: http_handler_test.go:671: HTTPHandlerTestSuite.TestGetMVCCNotFound	5.280s
PASS: http_handler_test.go:1019: HTTPHandlerTestSuite.TestGetSchema	5.290s
PASS: http_handler_test.go:1121: HTTPHandlerTestSuite.TestGetSchemaStorage	5.303s
PASS: http_handler_test.go:1000: HTTPHandlerTestSuite.TestGetSettings	5.291s
PASS: http_handler_test.go:581: HTTPHandlerTestSuite.TestGetTableMVCC	5.286s
PASS: http_handler_test.go:346: HTTPHandlerTestSuite.TestListTableRanges	5.283s
PASS: http_handler_test.go:322: HTTPHandlerTestSuite.TestListTableRegions	5.281s
PASS: http_handler_test.go:284: HTTPHandlerTestSuite.TestRangesAPI	5.277s
PASS: http_handler_test.go:229: HTTPHandlerTestSuite.TestRegionsAPI	5.275s
PASS: http_handler_test.go:250: HTTPHandlerTestSuite.TestRegionsAPIForClusterIndex	5.272s
PASS: http_handler_test.go:686: HTTPHandlerTestSuite.TestTiFlashReplica	7.432s
PASS: http_handler_test.go:1426: HTTPHandlerTestSuite.TestZipInfoForSQL	10.260s
PASS: statistics_handler_test.go:88: testDumpStatsSuite.TestDumpStatsAPI	3.924s
PASS: tidb_test.go:1381: tidbTestSuite.TestGracefulShutdown	2.791s
PASS: conn_test.go:756: ConnTestSuite.TestTiFlashFallback	5.716s
PASS: conn_test.go:587: ConnTestSuite.TestConnExecutionTimeout	12.410s
PASS: tidb_test.go:1490: tidbTestTopSQLSuite.TestTopSQLCPUProfile	4.988s

A lot of the tests are very slow, some are caused by the wait here

tidb/ddl/ddl.go

Lines 691 to 696 in e7d7371

func delayForAsyncCommit() {
cfg := config.GetGlobalConfig().TiKVClient.AsyncCommit
duration := cfg.SafeWindow + cfg.AllowedClockDrift
logutil.BgLogger().Info("sleep before DDL finishes to make async commit and 1PC safe",
zap.Duration("duration", duration))
time.Sleep(duration)

Here it sleeps about 2.5s, in the test code it doesn't really matter to sleep or not.

B.T.W, the real solution to make CI faster is to set a time limit for a single test cases
#26289

If we skip the tests that runs longer than 2s, we can finish the test much faster, and this is how long it really should be:

ok      github.com/pingcap/tidb/server  13.588s

If we want to archive the goal of finishing the whole unit test within 3min, we can't just ask some colleague to optimize it.

"Hey, our CI is slow, could you help me to optimize it a bit?"

This is not sustainable, sooner or later it will slow down again.

#26289 is a better solution, under that limitation, if a test case can not finish within 3s, the author should optimize it, otherwise move it out of the unit test. Maybe a integration test is better place for such case.

@zhouqiang-cl @tisonkun

Even better if we can monitor the CI time cost and have a dashboard for it, so we can see how it changes.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • [] No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

  • No release note

@ti-chi-bot ti-chi-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 26, 2021
server/server_test.go Outdated Show resolved Hide resolved
server/server_test.go Outdated Show resolved Hide resolved
@tisonkun
Copy link
Contributor

#26289 is a better solution, under that limitation, if a test case can not finish within 3s, the author should optimize it, otherwise move it out of the unit test. Maybe a integration test is better place for such case.

@zhouqiang-cl @tisonkun

Even better if we can monitor the CI time cost and have a dashboard for it, so we can see how it changes.

Yep. I post a script that we might build a fixture for timeout under #26289. For report, I'd suggest add a ci task to generate from go test -json.

JUnit provides a nice example which Golang ecosystem lack of.

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

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • AilinKid
  • 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 submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@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 Jul 27, 2021
@xhebox
Copy link
Contributor

xhebox commented Jul 27, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 60f613b

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 27, 2021
@ti-chi-bot ti-chi-bot merged commit 0aee65b into pingcap:master Jul 27, 2021
@tiancaiamao tiancaiamao deleted the server-ci-faster branch July 27, 2021 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/sql-infra SIG: SQL Infra size/XS Denotes a PR that changes 0-9 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
Development

Successfully merging this pull request may close these issues.

7 participants