cli: make the quit command more robust

[andreimatei]

Fixes #14184

The quit command send a streaming RPC and expects the connection to
drop. Except it wasn't handling the case where the connection dropped
before the client was even returning from the RPC call.

[cockroach-teamcity]

This change is 

[tamird]

Well I'm not sure how this fixes #14184, but lgtm otherwise.

---

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/cli/start.go, line 650 at r1 (raw file):

	// and that's all that this function is interested in.
	for {
		if _, err := stream.Recv(); err != nil {

what error are you expecting here?

---

Comments from Reviewable

[andreimatei]

I hope it fixes it because the failures I saw were the "connection is fucked" errors received by the grpc call itself (as opposed to received by stream.Recv, where we expect them).
Did you see... other failures?

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/cli/start.go, line 650 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

what error are you expecting here?

I'm not expecting any errors... Other than io.EOF. But I'm ignoring all errors anyway.
The point of this code is simply to consume the stream; I'm assuming that gRPC would like me to do that.

---

Comments from Reviewable

[tamird]

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/cli/start.go, line 650 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I'm not expecting any errors... Other than io.EOF. But I'm ignoring all errors anyway.
The point of this code is simply to consume the stream; I'm assuming that gRPC would like me to do that.

why is this loop not infinite? Does the remote hang up?

---

Comments from Reviewable

[a-robinson]

---

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

---

pkg/cli/start.go, line 645 at r1 (raw file):

	})
	if err != nil {
		return errors.Wrap(err, "Failed to connect to the node: error sending drain request")

Minor, but is the "error sending drain request" part of this message actually clarifying anything for anyone?

---

pkg/cli/start.go, line 650 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

why is this loop not infinite? Does the remote hang up?

I'm also curious about this. Shouldn't we just close the stream and return rather than receiving on it?

---

Comments from Reviewable

[andreimatei]

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

---

pkg/cli/start.go, line 645 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Minor, but is the "error sending drain request" part of this message actually clarifying anything for anyone?

ummm I dunno... It gives some info about the context; I'll keep it.

---

pkg/cli/start.go, line 650 at r1 (raw file):
The RPC server returns from the RPC handler, so at that point this guy will get either an EOF or an error.

Shouldn't we just close the stream and return rather than receiving on it?

Apparently there's no "close the stream". With a "client stream" you can close the send direction, but not the receive direction (I have no idea why there's a CloseSend()...). What you're supposed to do if you don't care about the result of a server-side streaming RPC is create a cancellable context and cancel it. But I don't think it's worth doing here.

---

Comments from Reviewable

[tamird]

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

---

pkg/cli/start.go, line 650 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

The RPC server returns from the RPC handler, so at that point this guy will get either an EOF or an error.

Shouldn't we just close the stream and return rather than receiving on it?

Apparently there's no "close the stream". With a "client stream" you can close the send direction, but not the receive direction (I have no idea why there's a CloseSend()...). What you're supposed to do if you don't care about the result of a server-side streaming RPC is create a cancellable context and cancel it. But I don't think it's worth doing here.

We should at least log if we get a non EOF error here.

---

Comments from Reviewable

[andreimatei]

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

---

pkg/cli/start.go, line 650 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

We should at least log if we get a non EOF error here.

Done.

---

Comments from Reviewable

[tamird]

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

---

Comments from Reviewable