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

tidb-server, terror: add cleanup when createServer exits abnormally #6947

Merged
merged 5 commits into from
Jul 3, 2018
Merged

tidb-server, terror: add cleanup when createServer exits abnormally #6947

merged 5 commits into from
Jul 3, 2018

Conversation

zimulala
Copy link
Contributor

What have you changed? (mandatory)

Handle #6946

What are the type of the changes (mandatory)?

  • Improvement

How has this PR been tested (mandatory)?

Before this PR：

2018/06/30 09:55:23.368 server.go:190: [warning] Secure connection is NOT ENABLED
2018/06/30 09:55:23.368 terror.go:327: [fatal] listen tcp 0.0.0.0:4000: bind: address already in use
/.../src/github.com/pingcap/tidb/server/server.go:173:

After this PR：

2018/06/30 09:55:31.783 server.go:190: [warning] Secure connection is NOT ENABLED
2018/06/30 09:55:31.785 delete_range.go:110: [info] [ddl] closing delRange session pool
2018/06/30 09:55:31.785 ddl_worker.go:80: [info] [ddl] close DDL worker 0
2018/06/30 09:55:31.785 ddl.go:382: [info] [ddl] closing DDL:8ab4f703-ed52-43c4-8dff-406b50aa39da takes time 344.571µs
2018/06/30 09:55:31.785 ddl.go:334: [info] [ddl] stop DDL:8ab4f703-ed52-43c4-8dff-406b50aa39da
2018/06/30 09:55:31.787 domain.go:652: [info] [stats] init stats info takes 4.577364ms
2018/06/30 09:55:31.787 domain.go:401: [info] [domain] close
2018/06/30 09:55:31.788 terror.go:335: [fatal] listen tcp 0.0.0.0:4000: bind: address already in use
/.../src/github.com/pingcap/tidb/server/server.go:173:

shenli
shenli reviewed Jun 30, 2018
View reviewed changes
terror/terror.go Outdated
@@ -328,6 +328,14 @@ func MustNil(err error) {
}
}

// CleanNotNil cleans up and fatals if err is not nil.
Copy link
Member

Choose a reason for hiding this comment

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

Why put it in the terror package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Becuase MustNil is here.

Copy link
Member

@coocood coocood Jul 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

How about we reuse the MustNil function, and change it like

func MustNil(err error, closeFn ...func())

@shenli
Copy link
Member

shenli commented Jun 30, 2018

rest LGTM

@zimulala
Copy link
Contributor Author

zimulala commented Jul 2, 2018

/run-all-tests

shenli
shenli reviewed Jul 2, 2018
View reviewed changes
tidb-server/main.go Outdated
@@ -487,11 +488,15 @@ func runServer() {
}
}

func close() {
Copy link
Member

Choose a reason for hiding this comment

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

s/close/closeDomainAndStoreage/

@zimulala
Copy link
Contributor Author

zimulala commented Jul 2, 2018

PTAL @shenli @coocood

@shenli
Copy link
Member

shenli commented Jul 2, 2018

LGTM

@shenli
Copy link
Member

shenli commented Jul 2, 2018

/run-all-tests

@shenli
Copy link
Member

shenli commented Jul 2, 2018

Do not forget to attach labels to your PR and cherry-pick this to the release-2.0 branch.

@shenli shenli added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 2, 2018
@shenli
Copy link
Member

shenli commented Jul 2, 2018

@coocood @tiancaiamao PTAL

@coocood
Copy link
Member

coocood commented Jul 3, 2018

LGTM

@coocood coocood added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 3, 2018
@zimulala zimulala merged commit f6ee36e into pingcap:master Jul 3, 2018
@zimulala zimulala deleted the add-cleanup branch July 3, 2018 06:20
zimulala added a commit to zimulala/tidb that referenced this pull request Jul 3, 2018
@zimulala
tidb-server, terror: add cleanup when createServer exits abnormally (…
a8bbd07 
…pingcap#6947)

* tidb-server, terror: add cleanup in createServer
@zimulala zimulala mentioned this pull request Jul 3, 2018
Merged
shenli pushed a commit that referenced this pull request Jul 3, 2018
@zimulala @shenli
tidb-server, terror: add cleanup when createServer exits abnormally (…
d341598 
…#6947) (#6964)

* tidb-server, terror: add cleanup in createServer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shenli shenli shenli left review comments

@coocood coocood coocood approved these changes

Assignees
No one assigned
Labels
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.
3 participants
@zimulala @shenli @coocood