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

storage: avoid goroutine leaks in raft transport #6010

Merged
merged 1 commit into from
Apr 13, 2016
Merged

storage: avoid goroutine leaks in raft transport #6010

merged 1 commit into from
Apr 13, 2016

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Apr 12, 2016

Previously each snapshot would spawn a goroutine that would live for
the duration of the outgoing request channel being non-empty; under
high load, that may be an indefinite period of time.

This shortens the lifetime of the spawned goroutines to the duration
of a single outgoing snapshot each.

Fixes #6007.


This change is Reviewable

@bdarnell
Copy link
Contributor

LGTM


Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


storage/raft_transport.go, line 199 [r1] (raw file):
Add a comment explaining that we start two streams so that we can use one for snapshots without blocking regular traffic. (but we do want snapshots to queue up behind each other)


storage/raft_transport.go, line 220 [r1] (raw file):
We'll write two errors to errCh but we'll only ever read one of them. That should be fine given the channel buffering but it's a little surprising and probably deserves a comment.


Comments from Reviewable

Previously each snapshot would spawn a goroutine that would live for
the duration of the outgoing request channel being non-empty; under
high load, that may be an indefinite period of time.

This shortens the lifetime of the spawned goroutines to the duration
of a single outgoing snapshot each.

Fixes #6007.
@tamird
Copy link
Contributor Author

tamird commented Apr 13, 2016

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.


storage/raft_transport.go, line 199 [r1] (raw file):
Done.


storage/raft_transport.go, line 220 [r1] (raw file):
Done.


Comments from Reviewable

@tamird tamird merged commit 1f3df27 into cockroachdb:master Apr 13, 2016
@tamird tamird deleted the raft-transport-dont-leak branch April 13, 2016 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants