storage: fill the reservation before sending the snapshot response

[petermattis]

If we send the response before filling the reservation the RPC can be
sent over the network waking up the goroutine which initiated the
preemptive snapshot and allowing it to perform another one before the
reservation is filled. This second preemptive snapshot would get
rejected and cause full replication to take much longer to achieve
resulting in test flakiness.

---

This change is

[petermattis]

This is ugly. I'm open to suggestions.

[tbg]

Could a sync.Once do the trick?

[petermattis]

Perhaps, though the synchronization sync.Once provides isn't necessary.

[petermattis]

Reworked this PR to have the snapshot sender wait for the stream EOF. PTAL.

[tamird]

---

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/storage/raft_transport.go, line 605 at r2 (raw file):

func (c snapshotClientWithBreaker) Recv() (*SnapshotResponse, error) {
  m, err := c.MultiRaft_RaftSnapshotClient.Recv()
  if err != nil && err != io.EOF {

please add a comment!

---

Comments from Reviewable

[petermattis]

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/storage/raft_transport.go, line 605 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

please add a comment!

What would the comment say? The code is pretty self-explanatory (at least, I thought it was). I'm going to merge because I'm heading out and this is causing serious flakiness, but I'll add a comment in a follow-on PR if we can decide on what it should say.

---

Comments from Reviewable

[vivekmenezes]

This is rather odd for two reasons 1. the resp is ignored, and 2. I imagine it would be part of a receiver loop

[tamird]

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

pkg/storage/raft_transport.go, line 605 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

What would the comment say? The code is pretty self-explanatory (at least, I thought it was). I'm going to merge because I'm heading out and this is causing serious flakiness, but I'll add a comment in a follow-on PR if we can decide on what it should say.

Eh, maybe you're right and this is fine.

---

pkg/storage/store.go, line 3094 at r2 (raw file):

Previously, vivekmenezes wrote…

This is rather odd for two reasons 1. the resp is ignored, and 2. I imagine it would be part of a receiver loop

I suppose we should log the response if we get something other than EOF.

---

Comments from Reviewable