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

embed: gracefully close peer handler #7879

Merged
merged 1 commit into from
May 8, 2017
Merged

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented May 4, 2017

Passing canceled context to close existing idle connections only once and to not delay the server shutdown. And make embed.Etcd.Peers non-exported (need release note).

c.f. #6174

@gyuho gyuho changed the title *: gracefully close peer http.Server embed: gracefully close peer http.Server May 5, 2017
@gyuho gyuho mentioned this pull request May 5, 2017
26 tasks
@gyuho gyuho changed the title embed: gracefully close peer http.Server embed: remove embed.Etcd.Peers field, gracefully close peer handler May 5, 2017
@gyuho gyuho changed the title embed: remove embed.Etcd.Peers field, gracefully close peer handler embed: remove Etcd.Peers field, gracefully close peer handler May 5, 2017
@gyuho gyuho added the WIP label May 5, 2017
@gyuho gyuho force-pushed the http-server branch 5 times, most recently from 140338e to 723d247 Compare May 5, 2017 15:15
@gyuho gyuho requested a review from heyitsanthony May 5, 2017 15:19
@gyuho gyuho added WIP and removed WIP labels May 5, 2017
embed/etcd.go Outdated
if plns[i], err = rafthttp.NewListener(u, &cfg.PeerTLSInfo); err != nil {
return nil, err
var l net.Listener
l, err = rafthttp.NewListener(u, &cfg.PeerTLSInfo)
Copy link
Contributor

Choose a reason for hiding this comment

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

l, lerr := to avoid the var l net.Listener?

embed/etcd.go Outdated
// gracefully shutdown http.Server; close open listeners, idle connections
// pass canceled context to close existing idle connections only once
// and not delay the server shutdown
serr := srv.Shutdown(canceled)
Copy link
Contributor

Choose a reason for hiding this comment

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

return srv.Shutdown(canceled)?

embed/etcd.go Outdated
@@ -51,7 +55,9 @@ const (

// Etcd contains a running etcd server and its listeners.
type Etcd struct {
Peers []net.Listener
peerServeFuncs []func() error
Copy link
Contributor

Choose a reason for hiding this comment

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

type Etcd struct {
    Peers []peerListener
    ...
}

...

type peerListener struct {
    net.Listener
    serve func() error
    close func(context.Context) error
}

?

@gyuho gyuho changed the title embed: remove Etcd.Peers field, gracefully close peer handler embed: gracefully close peer handler May 5, 2017
@gyuho gyuho force-pushed the http-server branch 4 times, most recently from 3b6e852 to 823f9b3 Compare May 5, 2017 19:41
@gyuho
Copy link
Contributor Author

gyuho commented May 5, 2017

Apparently this breaks in e2e. Will investigate.

EXPECT_DEBUG=1 go test -v -run TestCtlV3AuthMemberRemove
EXPECT_DEBUG=1 go test -v -run TestCtlV3MemberRemove

Both pass withNoStrictReconfig

@gyuho gyuho force-pushed the http-server branch 6 times, most recently from ce2e79c to 1367d54 Compare May 6, 2017 14:43
@gyuho
Copy link
Contributor Author

gyuho commented May 8, 2017

All fixed (it had to start rafthttp layer first).

PTAL.

@gyuho gyuho merged commit 3a2e765 into etcd-io:master May 8, 2017
@gyuho gyuho deleted the http-server branch May 8, 2017 21:00
@@ -226,12 +260,18 @@ func startPeerListeners(cfg *Config) (plns []net.Listener, err error) {
plog.Warningf("The scheme of peer url %s is HTTP while client cert auth (--peer-client-cert-auth) is enabled. Ignored client cert auth for this url.", u.String())
}
}
if plns[i], err = rafthttp.NewListener(u, &cfg.PeerTLSInfo); err != nil {
peers[i] = &peerListener{close: func(context.Context) error { return nil }}

Choose a reason for hiding this comment

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

Just curious, why are we initializing the close function here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants