server: return results of ongoing queries when graceful shutdown #19669

SunRunAway · 2020-09-01T11:55:02Z

What problem does this PR solve?

Issue Number: close #19663

Problem Summary:

Got Query execution was interrupted when graceful shutdown

What is changed and how it works?

Proposal: xxx

What's Changed:
Should not return ErrQueryInterrupted when server state is connStatusWaitShutdown. Results of ongoing queries should be returned successfully when graceful shutdown.

How it Works:

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

Manual test (add detailed scripts or steps below)

Add an e2e test:

master:

$ make
cd /Users/sunrunaway/code/gopath/src/github.com/pingcap/tidb && \
	CGO_ENABLED=1 GO111MODULE=on go build  -tags codes -ldflags '-X "github.com/pingcap/parser/mysql.TiDBReleaseVersion=v4.0.0-beta.2-1669-gb3f8be7d3" -X "github.com/pingcap/tidb/util/versioninfo.TiDBBuildTS=2020-11-30 09:26:36" -X "github.com/pingcap/tidb/util/versioninfo.TiDBGitHash=b3f8be7d3c9a719d043f41db14b62d259a79ecb2" -X "github.com/pingcap/tidb/util/versioninfo.TiDBGitBranch=master" -X "github.com/pingcap/tidb/util/versioninfo.TiDBEdition=Community"' -o /Users/sunrunaway/code/gopath/src/github.com/pingcap/tidb/tests/graceshutdown/bin/tidb-server tidb-server/main.go
Build TiDB Server successfully!
$ ./run-tests.sh
INFO[0000] starting tidb: bin/tidb-server --store=mocktikv --path=/tmp/tidb_gracefulshutdown/mocktikv -P=5501 --status=8500 --log-file=/tmp/tidb_gracefulshutdown/tidb5501.log 
INFO[0000] connect to server 127.0.0.1:5501 ok          
INFO[0001] service "tidb" Interrupt                     

----------------------------------------------------------------------
FAIL: graceshutdown_test.go:113: TestGracefulShutdownSuite.TestGracefulShutdown

graceshutdown_test.go:144:
    c.Assert(err, IsNil)
... value *mysql.MySQLError = &mysql.MySQLError{Number:0x525, Message:"Query execution was interrupted"} ("Error 1317: Query execution was interrupted")

OOPS: 0 passed, 1 FAILED
--- FAIL: TestGracefulShutdown (3.54s)
FAIL
exit status 1
FAIL	graceshutdown	3.557s

This branch:

$ make
cd /Users/sunrunaway/code/gopath/src/github.com/pingcap/tidb && \
	CGO_ENABLED=1 GO111MODULE=on go build  -tags codes -ldflags '-X "github.com/pingcap/parser/mysql.TiDBReleaseVersion=v4.0.0-beta.2-1671-g77e9fc4ad" -X "github.com/pingcap/tidb/util/versioninfo.TiDBBuildTS=2020-11-30 09:21:36" -X "github.com/pingcap/tidb/util/versioninfo.TiDBGitHash=77e9fc4ada3d6266f7d8e8702574f4c8d261e794" -X "github.com/pingcap/tidb/util/versioninfo.TiDBGitBranch=issue19663" -X "github.com/pingcap/tidb/util/versioninfo.TiDBEdition=Community"' -o /Users/sunrunaway/code/gopath/src/github.com/pingcap/tidb/tests/graceshutdown/bin/tidb-server tidb-server/main.go
Build TiDB Server successfully!
$ ./run-tests.sh
INFO[0000] starting tidb: bin/tidb-server --store=mocktikv --path=/tmp/tidb_gracefulshutdown/mocktikv -P=5501 --status=8500 --log-file=/tmp/tidb_gracefulshutdown/tidb5501.log 
INFO[0000] connect to server 127.0.0.1:5501 ok          
INFO[0001] service "tidb" Interrupt                                          
PASS: graceshutdown_test.go:113: TestGracefulShutdownSuite.TestGracefulShutdown	3.538s
OK: 1 passed
PASS
ok  	graceshutdown	3.549s

Release note

return results of ongoing queries when graceful shutdown.

SunRunAway · 2020-09-01T12:20:44Z

Should cherry-pick to release-4.0 and release-3.0 manually.

tiancaiamao

Results of ongoing queries should be returned successfully when graceful shutdown.

This may affect the correctness! we can either return a full of data with success, or an error.
When the query is interrupted, we can not be sure the integrity of the data.

tiancaiamao · 2020-09-03T12:36:09Z

server/server.go

@@ -486,9 +486,6 @@ func (s *Server) ShowProcessList() map[uint64]*util.ProcessInfo {
 	defer s.rwlock.RUnlock()
 	rs := make(map[uint64]*util.ProcessInfo, len(s.clients))
 	for _, client := range s.clients {
-		if atomic.LoadInt32(&client.status) == connStatusWaitShutdown {
-			continue


Somebody once complained that he use kill tidb connection XXX, but could still see the session in show processlist.
Here is a fix for him.

I think it should stay in processlist because it is really running and not fully closed. Maybe we can show a closing state for him.

SunRunAway · 2020-09-07T07:59:33Z

we can not be sure the integrity of the data.

The client will get a connection reset by peer error if the data is not fully received.

tiancaiamao · 2020-09-07T10:08:07Z

What I mean has nothing to do with 'connection reset by peer'.
We should return 100 lines, but we returns 50 lines, and we tell the client OK, that's not acceptable.

~~We should return 100 lines, but we can't guarantee the integrity of these data, so we return Interrupted error, that's more reasonable.~~

tiancaiamao · 2020-09-09T06:33:42Z

Graceful shutdown does not set the Killed flag, so the data are fully drained.
(Even the Killed flag is set, the Next() function will return an error).
We can ensure the integrity of the data, so it does not involve correctness issue.

From the client's point of view, when the server enter graceful shutdown mode, it's better for the ongoing connection to reply a success before it's closed, rather than replying a interrupted error and close.
So after the change, the behavior is more reasonable.

tiancaiamao · 2020-09-09T06:33:49Z

LGTM

ti-srebot · 2020-09-11T06:34:32Z

@tiancaiamao, @lysu, @zz-jason, @qw4990, PTAL.

ti-srebot · 2020-09-13T06:34:32Z

@tiancaiamao, @lysu, @zz-jason, @qw4990, PTAL.

ti-srebot · 2020-09-16T06:35:06Z

@tiancaiamao, @lysu, @zz-jason, @qw4990, PTAL.

zz-jason

how about adding some tests?

server/conn.go

ti-srebot · 2020-09-19T09:17:05Z

@tiancaiamao, @zz-jason, @lysu, @qw4990, PTAL.

ti-srebot · 2020-09-21T09:17:06Z

@tiancaiamao, @zz-jason, @lysu, @qw4990, PTAL.

ti-srebot · 2020-09-24T11:01:05Z

@tiancaiamao, @zz-jason, @qw4990, @lysu, PTAL.

ghost · 2020-09-24T18:17:21Z

There is no reward for this challenge pull request, so you can request a reward from @qw4990.

More

Tip : About reward you can refs to reward-command.

Warning: None

ti-srebot · 2020-09-26T11:01:34Z

@tiancaiamao, @zz-jason, @qw4990, @lysu, PTAL.

ti-srebot · 2020-09-29T11:02:27Z

@tiancaiamao, @zz-jason, @qw4990, PTAL.

qw4990

Almost LGTM, Please add some tests.

ti-srebot · 2020-10-14T07:58:52Z

@tiancaiamao, @zz-jason, @qw4990, PTAL.

ti-srebot · 2020-10-16T07:58:40Z

@tiancaiamao, @zz-jason, @qw4990, PTAL.

ti-srebot · 2020-10-19T07:58:48Z

@tiancaiamao, @zz-jason, @qw4990, PTAL.

tests/graceshutdown/graceshutdown_test.go

qw4990

LGTM

qw4990 · 2020-11-30T11:40:31Z

/merge

ti-srebot · 2020-11-30T11:43:35Z

/run-all-tests

SunRunAway · 2020-11-30T11:53:31Z

can we move the contents in tests/graceshutdown/.gitignore to .gitignore?

I prefer keeping it in tests/graceshutdown/ so this directory is a complete and independent component.

ti-srebot · 2020-11-30T11:57:05Z

@SunRunAway merge failed.

ti-srebot · 2020-12-02T11:54:43Z

@tiancaiamao, @zz-jason, @qw4990, @ti-srebot, PTAL.

SunRunAway · 2020-12-03T02:42:12Z

/merge

ti-srebot · 2020-12-03T02:44:48Z

/run-all-tests

ti-srebot · 2020-12-03T03:04:50Z

/run-all-tests

ti-srebot · 2020-12-03T03:06:56Z

@SunRunAway merge failed.

SunRunAway · 2020-12-03T06:08:01Z

/merge

ti-srebot · 2020-12-03T06:08:04Z

Your auto merge job has been accepted, waiting for:

21185

ti-srebot · 2020-12-03T06:10:44Z

/run-all-tests

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2020-12-03T06:29:57Z

cherry pick to release-4.0 in PR #21464

…gcap#19669)

) (#21464)

SunRunAway requested review from tiancaiamao, qw4990 and lysu September 1, 2020 12:17

SunRunAway added type/bugfix This PR fixes a bug. component/server labels Sep 1, 2020

SunRunAway added the status/PTAL label Sep 1, 2020

tiancaiamao reviewed Sep 3, 2020

View reviewed changes

ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 9, 2020

SunRunAway requested a review from zz-jason September 10, 2020 09:41

zz-jason reviewed Sep 17, 2020

View reviewed changes

server/conn.go Show resolved Hide resolved

lysu removed their request for review September 28, 2020 02:55

qw4990 reviewed Oct 12, 2020

View reviewed changes

SunRunAway removed the status/PTAL label Oct 22, 2020

qw4990 reviewed Nov 30, 2020

View reviewed changes

tests/graceshutdown/graceshutdown_test.go Show resolved Hide resolved

qw4990 approved these changes Nov 30, 2020

View reviewed changes

ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Nov 30, 2020

ti-srebot approved these changes Nov 30, 2020

View reviewed changes

ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 30, 2020

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 30, 2020

Merge branch 'master' into issue19663

6e229ae

SunRunAway added the needs-cherry-pick-4.0 label Nov 30, 2020

Merge branch 'master' into issue19663

b72f6ce

Merge branch 'master' into issue19663

88a665c

Merge branch 'master' into issue19663

12b8e8e

SunRunAway merged commit d67a102 into pingcap:master Dec 3, 2020

SunRunAway deleted the issue19663 branch December 3, 2020 06:29

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Dec 3, 2020

cherry pick pingcap#19669 to release-4.0

074bb58

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Dec 3, 2020

server: return results of ongoing queries when graceful shutdown (#19669) #21464

Merged

asiafrank pushed a commit to asiafrank/tidb that referenced this pull request Dec 8, 2020

server: return results of ongoing queries when graceful shutdown (pin…

5aa824d

…gcap#19669)

qw4990 pushed a commit that referenced this pull request Jan 27, 2021

server: return results of ongoing queries when graceful shutdown (#19669

c93141f

) (#21464)

server: return results of ongoing queries when graceful shutdown #19669

server: return results of ongoing queries when graceful shutdown #19669

Uh oh!

Conversation

SunRunAway commented Sep 1, 2020 • 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?

Related changes

Check List

Release note

Uh oh!

SunRunAway commented Sep 1, 2020

Uh oh!

tiancaiamao left a comment • 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!

tiancaiamao Sep 3, 2020

Choose a reason for hiding this comment

Uh oh!

SunRunAway Sep 7, 2020 • 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!

SunRunAway commented Sep 7, 2020

Uh oh!

tiancaiamao commented Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiancaiamao commented Sep 9, 2020

Uh oh!

tiancaiamao commented Sep 9, 2020

Uh oh!

ti-srebot commented Sep 11, 2020

Uh oh!

ti-srebot commented Sep 13, 2020

Uh oh!

ti-srebot commented Sep 16, 2020

Uh oh!

zz-jason left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ti-srebot commented Sep 19, 2020

Uh oh!

ti-srebot commented Sep 21, 2020

Uh oh!

ti-srebot commented Sep 24, 2020

Uh oh!

ghost commented Sep 24, 2020

Uh oh!

ti-srebot commented Sep 26, 2020

Uh oh!

ti-srebot commented Sep 29, 2020

Uh oh!

qw4990 left a comment

Choose a reason for hiding this comment

Uh oh!

ti-srebot commented Oct 14, 2020

Uh oh!

ti-srebot commented Oct 16, 2020

Uh oh!

ti-srebot commented Oct 19, 2020

Uh oh!

Uh oh!

qw4990 left a comment

Choose a reason for hiding this comment

Uh oh!

qw4990 commented Nov 30, 2020

Uh oh!

ti-srebot commented Nov 30, 2020

Uh oh!

SunRunAway commented Nov 30, 2020

Uh oh!

ti-srebot commented Nov 30, 2020

Uh oh!

ti-srebot commented Dec 2, 2020

Uh oh!

SunRunAway commented Dec 3, 2020

SunRunAway commented Sep 1, 2020 •

edited

Loading

tiancaiamao left a comment •

edited

Loading

SunRunAway Sep 7, 2020 •

edited

Loading

tiancaiamao commented Sep 7, 2020 •

edited

Loading