Rework goroutines and synchronization #136

ongardie-sfdc · 2016-07-12T01:02:39Z

Today, the division of work and the synchronization between goroutines gets to be hard to follow in places. I think we can do better, to make the library more maintainable and eliminate potential race conditions from accidentally shared state. Ideally, it'll become more unit testable too.

This commit includes a diagram and description of where I think we should go. I'm open to feedback on it. Some of it's probably underspecified, with details to be determined as we implement more; questions are fair game too.

I held back on subdividing the main Raft module into a nonblocking goroutine and blocking helpers, but it's something we could consider. I haven't studied the code enough to know whether that'd be feasible or advantageous.

The transition from here to there is going to take significant effort. Here are a few of the major differences:

Peer is structured completely differently from replication.go today.
Peer handles all communication including RequestVote, not just AppendEntries/InstallSnapshot as replication.go does today.
Fewer locks and shared state. commitment.go and raftstate.go remove locking/atomics, possibly merge into raft.go. Other goroutines don't get a handle to the Raft module's state.
Snapshots are created through a different flow.

I started on the replication.go/peer.go changes, but it was before I had a good idea of where things were heading. I'll be happy to pick that up again later.

/cc @superfell @cstlee @bmizerany @kr @slackpad @sean- #84

…onal)

slackpad · 2016-07-13T22:24:01Z

@ongardie-sfdc thanks for putting this together! I'm still wrapping my head around a few things (and traveling) so I'll have some more thorough feedback in a bit, but here are a couple of initial impressions:

The split of responsibilities and channel-based interfaces will be huge for testability, especially for cases that we currently can't drive things into today; we will now be able to inject whatever we want to these various channels. I'm super excited about this aspect of the design.

The short-lived goroutines kind of put me off initially as a performance smell but I see how they lead to nice, clean interactions with Peer non-blocking loop and as a checkpoint for the term check in the middle, etc. I'm thinking we might be able to create some pools of goroutines that we reuse and have that also be part of our work rate limiting. I need to think on these changes in general and how they can affect performance, and how pipelining will look. This might be a bogus worry as well, need to think more on it :-)

This will definitely be a large effort and will take some time. In the near term I'd like to try to ship a release candidate version of Consul with the #84 peer changes. This design will address several of our outstanding issues (especially the AppendEntries stale term checks) so I'm trying to weigh if it's practical to clean those up as simply as possible in the short term in the existing architecture and target this into a different, longer term integration branch to roll out in a later release of Consul.

kr · 2016-07-14T22:59:16Z

we might be able to create some pools of goroutines that we reuse

Definitely measure that approach before adopting it. Making a new goroutine is pretty cheap; can be cheaper than the alternative. For example:

https://github.com/golang/net/blob/f841c39d/http2/server.go#L610-L617
https://github.com/golang/net/blob/f841c39d/http2/server.go#L848-L851

This was a surprising result; this way turned out faster than sending frames to a long-running goroutine to be written.

as a performance smell … need to think more on it

For performance, I'd strongly encourage an empirical approach over intuition or even careful thought. You'll be surprised how hard it is to predict where the performance problems are (and aren't!). 😄

ongardie · 2016-07-18T05:00:45Z

I've now reworked the replication/peer side of things, but I need to get the existing tests passing again. And then there's adding actual unit tests now that that's possible. I'll post for a review one of these days. Pipelining fit in easily using the Transport's future return value, not the additional channel as before.

So regarding short-term shipping vs making this change, I think the biggest part is already mostly done. The rest will be more incremental, like removing dependencies on shared Raft/RaftState state from other parts of the library.

slackpad · 2016-07-18T15:29:12Z

@ongardie if you've got a branch some place I'm definitely willing to take a look!

ongardie-sfdc · 2016-07-20T18:49:49Z

Well, this patch is about ready for review, but the project might feel less 1337 now.

18 files changed, 2064 insertions(+), 1337 deletions(-)

Despite that, I'll post it for review this afternoon.

See PR#136 for rationale

ongardie-sfdc · 2016-07-20T20:27:40Z

commands.go

-
-	// There are scenarios where this request didn't succeed
-	// but there's no need to wait/back-off the next attempt.
-	NoRetryBackoff bool


Removed this because I don't think there are legitimate scenarios where the leader should back off upon receiving a reply. Backoff happens now only in response to transport errors.

Yeah this is an interesting one. The types of things where we didn't set this to true were were kind of a miscellaneous grab of application errors like not being able to write to the log. I'd probably rather see the back pressure done as a delayed response (such as if we did retries writing to the log, etc.) vs. an immediate response asking for a back off. Removing this and keeping it for comm errors seems like the way to go.

How important is backward compatibility? If providing backward compatibility, perhaps it would be better to retain this field but document that it's deprecated and ignored.

@rogpeppe we've got a number of non-compatible changes in this version of the library already, though they are generally pretty easy to deal with (it took about 2 hours to get Consul building and booting again). We intend to keep this in a branch and once it's done we will message the community and give a little bit of time for people to adapt to the changes before we take it to master. Whenever we can make something safer (like ID and addresses as separate types) or simpler we are breaking things. We've also got a story for how folks can interoperate with newer versions of the library during the transition - https://github.com/hashicorp/raft/blob/library-v2-stage-one/config.go#L10-L115.

ongardie-sfdc · 2016-07-20T21:01:23Z

Sorry for the many emails. Blame GitHub.

This is ready for your review now, @slackpad @sean- @superfell and any other generous folks.

ongardie-sfdc · 2016-07-21T16:39:02Z

peer.go

+// blockingSelect reads/writes the Peer channels just once, blocking if needed.
+func (p *peerState) blockingSelect() {
+	// We need to send a heartbeat at lastHeartbeatSent + heartbeatInterval.
+	heartbeatTimer := time.After(p.shared.options.heartbeatInterval -


Need an if statement around this so that Followers and Candidates don't fire all the time.

slackpad · 2016-07-26T16:00:26Z

inmem_transport.go

@@ -10,7 +10,7 @@ import (
 // NewInmemAddr returns a new in-memory addr with
 // a randomly generate UUID as the ID.
 func NewInmemAddr() ServerAddress {
-	return ServerAddress(generateUUID())
+	return ServerAddress(generateUUID()[:4])


It's definitely the best part of this patch.

ongardie-sfdc · 2016-08-19T19:53:57Z

With de57f4d, Peer still assumes the pipeline won't fail, and a new "reliable pipeline" wrapper handles errors under the hood.

ongardie-sfdc · 2016-08-19T19:57:34Z

Hey I just discovered this thing where net_transport.go tries to detect a heartbeat based on it being all zeros (no longer true with this PR), then directly invokes a function that's probably not safe to run concurrently. I think we should disable it for now and revisit later.

ongardie-sfdc · 2016-08-19T21:20:05Z

^@slackpad please add to issue-84 checklist

slackpad · 2016-08-19T22:37:31Z

@ongardie-sfdc added as item 25

ongardie-sfdc · 2016-08-29T16:46:25Z

hey @slackpad @sean-, any ETA on reviewing this?

slackpad · 2016-08-29T17:07:33Z

Sorry for the delay @ongardie-sfdc - I will review this week!

slackpad · 2016-09-01T23:23:05Z

api.go

@@ -372,6 +382,7 @@ func (r *Raft) restoreSnapshot() error {
 			r.configurations.latest = configuration
 			r.configurations.latestIndex = snapshot.Index
 		}
+		r.updatePeers()


This is going to get called on line 332 in NewRaft() is there a good reason to call it here? We might change the configuration later when we look through the log entries.

slackpad · 2016-09-02T04:15:17Z

Made it about 1/3 through but I'll be out tomorrow, so this'll spill over into next week. Things are looking really good so far. Will update once I pick up again (may have a little time on Saturday).

slackpad · 2016-09-13T21:54:27Z

peer.go

+	// If allowPipeline is true, pipelineUnsupported must be false.
+	allowPipeline bool
+
+	// Set to true when we're an AppendEntries request is being sent on the


Comment doesn't make sense.

slackpad · 2016-09-13T22:32:34Z

@ongardie-sfdc I've made a pass through everything and I think the basic structure of this is sound. Pointed out a couple tiny things in the comments.

Let's go ahead and rebase or merge with the latest branch contents and get this into issue-84-integration. Over there I'm going to make another super detailed pass through the peer logic and the unit tests, as well as try to get some benchmarks going that we can pick to the stage one branch to get a before/after.

slackpad · 2016-09-13T22:33:03Z

And sorry it took so long to get back to this!

ongardie-sfdc · 2016-09-15T22:39:42Z

Thanks @slackpad, that sounds good to me. I'll be on vacation next week and most of the following, but I'm going to fix up the little nits and then try to rebase or merge before I go.

ongardie-sfdc · 2016-09-15T23:00:34Z

I think 8a71deb is the last of my non-merge-related changes.

Notable merge issues include: - New RPCHeader field - New InstallSnapshotRequest.SnapshotVersion field - processConfigurationLogEntry function

ongardie-sfdc · 2016-09-16T00:31:04Z

Merged issue-84-integration into ongardie/modules. My confidence that I got that merge perfectly right isn't terribly high, but it's probably good enough to call done. Unit tests and a quick run seem ok.

slackpad · 2016-09-16T00:51:20Z

Cool I'll scan through it again for merge weirdness and bring it over to
the branch.

On Thu, Sep 15, 2016 at 5:31 PM, Diego Ongaro notifications@github.com
wrote:

Merged issue-84-integration into ongardie/modules. My confidence that I
got that merge perfectly right isn't terribly high, but it's probably good
enough to call done. Unit tests and a quick run seem ok.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#136 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AApG5az1eLA7_nw2zUXaAfFPbK09-4k4ks5qqeNKgaJpZM4JJ8Qv
.

ongardie · 2016-09-24T08:17:41Z

woot

Add INTERNALS.md to describe goroutines and synchronization (aspirati…

948510a

…onal)

Replace replication.go with peer.go

d3f3db0

See PR#136 for rationale

ongardie-sfdc reviewed Jul 20, 2016
View reviewed changes

ongardie-sfdc reviewed Jul 21, 2016
View reviewed changes

ongardie-sfdc mentioned this pull request Jul 21, 2016

Adds in-place upgrade and manual recovery support. #139

Merged

slackpad reviewed Jul 26, 2016
View reviewed changes

Add adapter for AppendEntries pipeline that reopens itself

de57f4d

ongardie mentioned this pull request Aug 22, 2016

Forcing a node to become leader without rebooting servers #151

Closed

slackpad reviewed Sep 1, 2016
View reviewed changes

ongardie added 4 commits September 6, 2016 16:08

Drop redundant updatePeers() call

6b4fa9f

Add comment to verifyFuture

83826d2

Fix typo in Stats()

a3fb4d6

Fix bug in immediately committed configuration not marked as such

09f8f6e

slackpad reviewed Sep 13, 2016
View reviewed changes

Small fixes following slackpad's review

8a71deb

Merge 'origin/issue-84-integration' into modules

b254ce0

Notable merge issues include: - New RPCHeader field - New InstallSnapshotRequest.SnapshotVersion field - processConfigurationLogEntry function

slackpad merged commit ebf12dd into hashicorp:issue-84-integration Sep 24, 2016

slackpad mentioned this pull request Oct 7, 2016

Clean up heartbeat detection / short circuit code #169

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework goroutines and synchronization #136

Rework goroutines and synchronization #136

ongardie-sfdc commented Jul 12, 2016

slackpad commented Jul 13, 2016

kr commented Jul 14, 2016

ongardie commented Jul 18, 2016

slackpad commented Jul 18, 2016

ongardie-sfdc commented Jul 20, 2016

ongardie-sfdc Jul 20, 2016

slackpad Jul 26, 2016

rogpeppe Aug 10, 2016

slackpad Sep 1, 2016

ongardie-sfdc commented Jul 20, 2016

ongardie-sfdc Jul 21, 2016

slackpad Jul 26, 2016

ongardie-sfdc Jul 26, 2016

ongardie-sfdc commented Aug 19, 2016

ongardie-sfdc commented Aug 19, 2016

ongardie-sfdc commented Aug 19, 2016

slackpad commented Aug 19, 2016

ongardie-sfdc commented Aug 29, 2016

slackpad commented Aug 29, 2016

slackpad Sep 1, 2016

slackpad commented Sep 2, 2016

slackpad Sep 13, 2016

slackpad commented Sep 13, 2016

slackpad commented Sep 13, 2016

ongardie-sfdc commented Sep 15, 2016

ongardie-sfdc commented Sep 15, 2016

ongardie-sfdc commented Sep 16, 2016

slackpad commented Sep 16, 2016

ongardie commented Sep 24, 2016

Rework goroutines and synchronization #136

Rework goroutines and synchronization #136

Conversation

ongardie-sfdc commented Jul 12, 2016

slackpad commented Jul 13, 2016

kr commented Jul 14, 2016

ongardie commented Jul 18, 2016

slackpad commented Jul 18, 2016

ongardie-sfdc commented Jul 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ongardie-sfdc commented Jul 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ongardie-sfdc commented Aug 19, 2016

ongardie-sfdc commented Aug 19, 2016

ongardie-sfdc commented Aug 19, 2016

slackpad commented Aug 19, 2016

ongardie-sfdc commented Aug 29, 2016

slackpad commented Aug 29, 2016

Choose a reason for hiding this comment

slackpad commented Sep 2, 2016

Choose a reason for hiding this comment

slackpad commented Sep 13, 2016

slackpad commented Sep 13, 2016

ongardie-sfdc commented Sep 15, 2016

ongardie-sfdc commented Sep 15, 2016

ongardie-sfdc commented Sep 16, 2016

slackpad commented Sep 16, 2016

ongardie commented Sep 24, 2016