server: close sql rows to fix unstable test by hawkingrei · Pull Request #30306 · pingcap/tidb

hawkingrei · 2021-12-01T07:08:34Z

Signed-off-by: Weizhen Wang wangweizhen@pingcap.com

What problem does this PR solve?

Problem Summary:

What is changed and how it works?

server: fix unstable test by removing parallel in test

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

Release note

None

ti-chi-bot · 2021-12-01T07:08:35Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

djshow832
mjonss

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.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

hawkingrei · 2021-12-01T07:13:33Z

Don't worry about that this PR is too big to review. It is just remove Parallel and remove those cases to tidb_serial_test.go

hawkingrei · 2021-12-01T07:23:17Z

when Parallel run test, The port of NewServer is not enough random so that it lead to fail to startup Server.

mjonss

LGTM

Just one question about a potentially missed t.Parallel()

server/tidb_serial_test.go

hawkingrei · 2021-12-01T08:29:47Z

@tisonkun @kennytm @wjhuang2016 PTAL

wjhuang2016

Please compare the test time for the server package.

hawkingrei · 2021-12-01T09:21:18Z

/run-check_dev_2

djshow832 · 2021-12-01T10:24:01Z

There are 3 more tests not covered: #30303, #30256, #30286. Are you going to file another PR for them?

djshow832 · 2021-12-01T10:25:52Z

TestTopSQLAgent fails in CI, which runs serially.

djshow832 · 2021-12-01T10:33:33Z

when Parallel run test, The port of NewServer is not enough random so that it lead to fail to startup Server.

createTidbTestSuite calls waitUntilServerOnline. If the server failed to startup, I don't understand why there is no log reporting failed to connect DB in every 10 ms.

mjonss · 2021-12-01T10:48:56Z

when Parallel run test, The port of NewServer is not enough random so that it lead to fail to startup Server.

createTidbTestSuite calls waitUntilServerOnline. If the server failed to startup, I don't understand why there is no log reporting failed to connect DB in every 10 ms.

Oh, waitUntilServerOnline (waitUntilServerCanConnect) does only do sql.Open, which does not do the connection, it would need db.Ping() to actually make the connection!

djshow832 · 2021-12-01T10:53:41Z

when Parallel run test, The port of NewServer is not enough random so that it lead to fail to startup Server.

createTidbTestSuite calls waitUntilServerOnline. If the server failed to startup, I don't understand why there is no log reporting failed to connect DB in every 10 ms.

Oh, waitUntilServerOnline (waitUntilServerCanConnect) does only do sql.Open, which does not do the connection, it would need db.Ping() to actually make the connection!

I see, I checked sql.Open and found it doesn't do the connection in the current goroutine.
I think we should modify waitUntilServerOnline to do the ping and restart a server if the ping fails. In this way, the CI still runs in parallel, which is faster.

djshow832 · 2021-12-01T11:10:27Z

Will net.Listen return an error if the port is occupied? @mjonss I guess so, but the case should fail if net.Listen fails:

if s.listener, err = net.Listen(tcpProto, addr); err != nil {
	return nil, errors.Trace(err)
}

server, err := NewServer(cfg, ts.tidbdrv)
require.NoError(t, err)

hawkingrei · 2021-12-01T11:58:07Z

/hold

server/server_test.go

server/tidb_test.go

server/http_handler_test.go

server/server_test.go

hawkingrei · 2021-12-06T11:17:02Z

@bb7133 @djshow832 PTAL

hawkingrei · 2021-12-06T11:18:09Z

server/tidb_test.go

TestSocket need a custom link checker.

server/server_test.go

server/statistics_handler_serial_test.go

server/tidb_serial_test.go

server/tidb_test.go

mjonss · 2021-12-06T12:18:11Z

Will net.Listen return an error if the port is occupied? @mjonss I guess so, but the case should fail if net.Listen fails:
if s.listener, err = net.Listen(tcpProto, addr); err != nil {
	return nil, errors.Trace(err)
}
server, err := NewServer(cfg, ts.tidbdrv)
require.NoError(t, err)

(for reference 😃 ) yes, if the Listen() call fail it will also fail to start the server:

[2021/12/06 13:15:45.137 +01:00] [FATAL] [main.go:660] ["failed to create the server"] [error="listen tcp 0.0.0.0:4000: bind: address already in use"] [stack="main.createServer\n\trepos/tidb/tidb-server/main.go:660\nmain.main\n\trepos/tidb/tidb-server/main.go:199\nruntime.main\n\t/usr/lib/go-1.17/src/runtime/proc.go:255"] [stack="main.createServer\n\trepos/tidb/tidb-server/main.go:660\nmain.main\n\trepos/tidb/tidb-server/main.go:199\nruntime.main\n\t/usr/lib/go-1.17/src/runtime/proc.go:255"]

mjonss

TestOnlySocket fails when I try to run it:

--- FAIL: TestOnlySocket (0.79s)
    server_test.go:848: 
        	Error Trace:	server_test.go:848
        	            				tidb_test.go:769
        	            				server_test.go:117
        	            				tidb_test.go:760
        	Error:      	Not equal: 
        	            	expected: "root@localhost"
        	            	actual  : ""
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-root@localhost
        	            	+
        	Test:       	TestOnlySocket

server/server_test.go

hawkingrei · 2021-12-06T12:53:44Z

TestOnlySocket fails when I try to run it:

--- FAIL: TestOnlySocket (0.79s)
    server_test.go:848: 
        	Error Trace:	server_test.go:848
        	            				tidb_test.go:769
        	            				server_test.go:117
        	            				tidb_test.go:760
        	Error:      	Not equal: 
        	            	expected: "root@localhost"
        	            	actual  : ""
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-root@localhost
        	            	+
        	Test:       	TestOnlySocket

it is a bug. I have fixed.

server/server_test.go

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

hawkingrei · 2021-12-07T02:21:24Z

@bb7133 @djshow832 @mjonss PTAL

bb7133

LGTM

bb7133

LGTM

bb7133 · 2021-12-07T03:54:51Z

/merge

ti-chi-bot · 2021-12-07T03:54:54Z

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

Details

Commit hash: abf158d

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 1, 2021

hawkingrei changed the title ~~server: fix unstable test by removing parallel in test~~ server: fix unstable test by removing parallel Dec 1, 2021

mjonss approved these changes Dec 1, 2021

View reviewed changes

server/tidb_serial_test.go Outdated Show resolved Hide resolved

ti-chi-bot added status/LGT1 Indicates that a PR has LGTM 1. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 1, 2021

hawkingrei force-pushed the remove_parallel branch 2 times, most recently from 4b9f974 to b6a2cc8 Compare December 1, 2021 08:07

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021

hawkingrei force-pushed the remove_parallel branch from b6a2cc8 to 88f2685 Compare December 1, 2021 08:16

hawkingrei force-pushed the remove_parallel branch from 88f2685 to 6fcd3f6 Compare December 1, 2021 08:46

wjhuang2016 reviewed Dec 1, 2021

View reviewed changes

hawkingrei force-pushed the remove_parallel branch from 6fcd3f6 to 5ee35b7 Compare December 1, 2021 08:58

djshow832 approved these changes Dec 1, 2021

View reviewed changes

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 Dec 1, 2021

djshow832 mentioned this pull request Dec 1, 2021

server: fix a unstable test TestInitConnect #30250

Closed

12 tasks

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 1, 2021

hawkingrei force-pushed the remove_parallel branch 2 times, most recently from 568a6cd to a1f8c02 Compare December 6, 2021 06:39

xhebox reviewed Dec 6, 2021

View reviewed changes

server/server_test.go Outdated Show resolved Hide resolved

bb7133 reviewed Dec 6, 2021

View reviewed changes

djshow832 reviewed Dec 6, 2021

View reviewed changes

server/http_handler_test.go Outdated Show resolved Hide resolved

djshow832 reviewed Dec 6, 2021

View reviewed changes

server/server_test.go Outdated Show resolved Hide resolved

djshow832 reviewed Dec 6, 2021

View reviewed changes

server/server_test.go Outdated Show resolved Hide resolved

bb7133 mentioned this pull request Dec 6, 2021

mockstore: panic happens for test TestMvccTxnRead #30440

Closed

hawkingrei commented Dec 6, 2021

View reviewed changes

server/tidb_test.go Outdated

Copy link

Member Author

hawkingrei Dec 6, 2021 •

edited

Loading

Choose a reason for hiding this comment

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

TestSocket need a custom link checker.

hawkingrei commented Dec 6, 2021

View reviewed changes

server/server_test.go Outdated Show resolved Hide resolved

djshow832 reviewed Dec 6, 2021

View reviewed changes

server/server_test.go Outdated Show resolved Hide resolved

server/server_test.go Outdated Show resolved Hide resolved

djshow832 reviewed Dec 6, 2021

View reviewed changes

server/statistics_handler_serial_test.go Outdated Show resolved Hide resolved

server/tidb_serial_test.go Outdated Show resolved Hide resolved

server/tidb_test.go Outdated Show resolved Hide resolved

mjonss reviewed Dec 6, 2021

View reviewed changes

server/server_test.go Outdated Show resolved Hide resolved

djshow832 reviewed Dec 6, 2021

View reviewed changes

server/server_test.go Outdated Show resolved Hide resolved

hawkingrei added 2 commits December 7, 2021 09:48

server: close sql rows to fix unstable test

eb2d2d7

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

Merge branch 'master' into remove_parallel

abf158d

bb7133 reviewed Dec 7, 2021

View reviewed changes

bb7133 approved these changes Dec 7, 2021

View reviewed changes

Merge branch 'master' into remove_parallel

bf31f24

This was referenced Dec 7, 2021

IT Unstable case TestSumAvg #30297

Closed

Unstable test found in CI TestInitConnect #30249

Closed

TestAllHistory failed in CI #30256

Closed

IT Unstable case TestGetIndexMVCC #30286

Closed

bb7133 mentioned this pull request Dec 16, 2021

Unstable CI test TestDBNameEscape #30303

Closed

Conversation

hawkingrei commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hawkingrei commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hawkingrei commented Dec 1, 2021

Uh oh!

mjonss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hawkingrei commented Dec 1, 2021

Uh oh!

wjhuang2016 left a comment

Choose a reason for hiding this comment

Uh oh!

hawkingrei commented Dec 1, 2021

Uh oh!

djshow832 commented Dec 1, 2021

Uh oh!

djshow832 commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djshow832 commented Dec 1, 2021

Uh oh!

mjonss commented Dec 1, 2021

Uh oh!

djshow832 commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djshow832 commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hawkingrei commented Dec 1, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkingrei commented Dec 6, 2021

Uh oh!

hawkingrei Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mjonss commented Dec 6, 2021

Uh oh!

mjonss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hawkingrei commented Dec 6, 2021

Uh oh!

Uh oh!

hawkingrei commented Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bb7133 left a comment

Choose a reason for hiding this comment

hawkingrei commented Dec 1, 2021 •

edited

Loading

ti-chi-bot commented Dec 1, 2021 •

edited

Loading

hawkingrei commented Dec 1, 2021 •

edited

Loading

djshow832 commented Dec 1, 2021 •

edited

Loading

djshow832 commented Dec 1, 2021 •

edited

Loading

djshow832 commented Dec 1, 2021 •

edited

Loading

hawkingrei Dec 6, 2021 •

edited

Loading

hawkingrei commented Dec 7, 2021 •

edited

Loading