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: close all connection directly when terminate tidb #8692

Merged
merged 6 commits into from
Dec 17, 2018

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Dec 14, 2018

What problem does this PR solve?

Close all connection directly when terminate tidb.
Currently, if TiDB got terminated signal, TiDB will call cancelFunc of the connection. But call cancelFunc will make the result be uncertainty to client.
So just close the connections and return error.

What is changed and how it works?

Check List

Tests

Code changes

Side effects

Related changes

  • Need to cherry-pick to the release branch

This change is Reviewable

@crazycs520
Copy link
Contributor Author

@winkyao @tiancaiamao PTAL

@crazycs520
Copy link
Contributor Author

/run-all-tests

@winkyao winkyao added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Dec 14, 2018
@@ -153,6 +153,10 @@ func (cc *clientConn) Close() error {
delete(cc.server.clients, cc.connectionID)
connections := len(cc.server.clients)
cc.server.rwlock.Unlock()
return closeConn(cc, connections)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will Close be called twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally won't.
https://github.com/pingcap/tidb/blob/master/server/conn.go#L458 There will check connection status to avoid double close.

server/conn.go Outdated Show resolved Hide resolved
@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 14, 2018
@winkyao
Copy link
Contributor

winkyao commented Dec 14, 2018

Please stop the schemaValidator in the beginning of the domain.Close.

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.

Rest LGTM

winkyao
winkyao previously approved these changes Dec 14, 2018
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@zhouqiang-cl
Copy link
Contributor

/rebuild

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

/run-unit-test

@crazycs520
Copy link
Contributor Author

/run-common-test

domain/domain.go Outdated Show resolved Hide resolved
@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520
Copy link
Contributor Author

/rebuild

@crazycs520
Copy link
Contributor Author

/rebuild

@crazycs520
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

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

LGTM

@crazycs520
Copy link
Contributor Author

/rebuild

@winkyao
Copy link
Contributor

winkyao commented Dec 17, 2018

/run-all-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/server priority/release-blocker This issue blocks a release. Please solve it ASAP. status/LGT1 Indicates that a PR has LGTM 1. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants