This change is 

NB: I'm planning to cherry-pick this to v1.1., though I can be convinced otherwise.

I'm -1 on cherry-picking to 1.1. If we make sure we have migrations for everything in 2.0, it's unnecessary. We currently allow upgrading from any 1.1.x release to 2.0.y; I'd rather not introduce a requirement that the cluster be upgraded to at least 1.1.5 before 2.0 unless we have to.

Reviewed 4 of 4 files at r1.
Review status: 3 of 4 files reviewed at latest revision, 2 unresolved discussions.

pkg/acceptance/version_upgrade_test.go, line 70 at r1 (raw file):

	// The answer is "not well". v1.1 has the same problem except in late patch
	// releases. v2.0 does not have this issue, but it should be tested once (if)
	// we introduce a new command to v2.1 by adapting the below code. See

So the idea behind this block is that the first new command in 2.1 should uncomment the ba.Add line with the new command to provide a test that it doesn't crash the old node?

pkg/server/node.go, line 835 at r1 (raw file):

	for _, req := range reqs {
		if req.GetValue() == nil {
			return log.Safe("unknown request in batch")

I think it would be best if this was a roachpb.Error so we can cleanly catch it on the client side (for example with RecomputeStats, we could simply fail silently whenever we get the old-version error)

Comments from Reviewable

Agreed. I was suggesting the cherry-pick mostly to be able to cash in on the ba.Add in the acceptance test earlier, but that has been removed anyway.

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

pkg/server/node.go, line 835 at r1 (raw file):

Good idea, done.

pkg/acceptance/version_upgrade_test.go, line 70 at r1 (raw file):

Yeah. I removed this though, doesn't seem to add anything given TestNodeSendUnknownBatchRequest.

Comments from Reviewable

LGTM

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

Comments from Reviewable