Add expected RangeDescriptor to AdminChangeReplicas and adopt

[ajwerner]

This PR comes in two commits. The first updates the roachpb api and the second adopts the update. This is the first of the PRs which will address #36025. See the commit messages for more details.

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 2 of 2 files at r1, 11 of 11 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

---

pkg/internal/client/db.go, line 509 at r2 (raw file):

		return nil, err
	}
	responses := b.RawResponse().Responses

nit: I would change this to be b.response.Responses, which is what Batch.fillResults uses. This is all a little bit of an abuse because we should be generating this response in Batch.fillResults instead given that this is not a "raw" request.

Another option would be to add another response variant to Result. I'm not sure what type you'd give it (other interface{}?), but would fit in with the current approach of some responses populating Result.Rows and others populating Result.Keys. Then you could populate this in Batch.fillResults and pull the result in db.AdminChangeReplicas from getOneResult.

---

pkg/roachpb/api.proto, line 688 at r2 (raw file):

  RequestHeader header = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];
  ReplicaChangeType change_type = 2;
  repeated ReplicationTarget targets = 3 [(gogoproto.nullable) = false];

We should explain the semantics of this field. Critically, mention that the sequence of changes will can be partially applied if it races with other replica movement, but will always be applied in order.

You might also consider adding a test similar to TestConcurrentAdminChangeReplicasRequests that constructs a sequence of replication targets such that there are multiple valid histories for two racing AdminChangeReplicasRequests. Something like:

origDesc = 1
actor 1: expDesc: 1, targets: [2, 3, 4]
actor 2: expDesc: 2, targets: [3, 2, 1]

You'd then assert that the final target is either 1 or 4.

---

pkg/storage/client_replica_test.go, line 2020 at r2 (raw file):

		if err == nil {
			numSuccesses++
		}

Let's assert that the only error we get is what we expect when the expected desc doesn't match.

---

pkg/storage/replica.go, line 1223 at r2 (raw file):

		}
		for _, target := range tArgs.Targets {
			expDesc, err = r.ChangeReplicas(

Could you add a comment about why we set expDesc to the result of each previous step?

---

pkg/storage/replica_command.go, line 1092 at r2 (raw file):

	}

	// After each call to the

This needs some love.

---

pkg/storage/replica_command.go, line 1116 at r2 (raw file):

	// updateRangeDesc updates the passed RangeDescriptor following the successful
	// completion of an AdminChangeReplicasRequest with	the single provided target

Strange tab here.

[ajwerner]

Thanks for the review!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/internal/client/db.go, line 509 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: I would change this to be b.response.Responses, which is what Batch.fillResults uses. This is all a little bit of an abuse because we should be generating this response in Batch.fillResults instead given that this is not a "raw" request.

Another option would be to add another response variant to Result. I'm not sure what type you'd give it (other interface{}?), but would fit in with the current approach of some responses populating Result.Rows and others populating Result.Keys. Then you could populate this in Batch.fillResults and pull the result in db.AdminChangeReplicas from getOneResult.

I went with the first suggestion. Muddying up Results further didn't feel clean .

---

pkg/roachpb/api.proto, line 688 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We should explain the semantics of this field. Critically, mention that the sequence of changes will can be partially applied if it races with other replica movement, but will always be applied in order.

You might also consider adding a test similar to TestConcurrentAdminChangeReplicasRequests that constructs a sequence of replication targets such that there are multiple valid histories for two racing AdminChangeReplicasRequests. Something like:

origDesc = 1
actor 1: expDesc: 1, target: [2, 3, 4]
actor 2: expDesc: 2, target: [3, 2, 1]

You'd then assert that the final target is either 1 or 4.

I added this but it needs to be

origDesc = 1
actor 1: expDesc: 1, target: [2, 3, 4]
actor 2: expDesc: 2, target: [3, 5]

Because it's an error to add an existing entry.

---

pkg/storage/client_replica_test.go, line 2020 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Let's assert that the only error we get is what we expect when the expected desc doesn't match.

Done. It also turns out this test was too pessimistic because it was written when I had the racing goroutines each do a read before they did their write but in that scenario multiple can succeed. It'd be fancy if I built some code to verify that valid history occurred. For now I made it such that it guarantees that only one succeeds and that it succeeds completely.

I also added the test you suggested above.

---

pkg/storage/replica_command.go, line 1092 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This needs some love.

Actually just stale, removed.

[nvanbenschoten]

Reviewed 2 of 13 files at r3, 11 of 11 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

[ajwerner]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)