WIP: allocator: support per-replica constraints & improve locality handling

[a-robinson]

This is the core work required for #19985 (but without exposing it to
users yet), and fixes #20751 along the way.

Release note: None

It still needs some more love -- the code's a bit rough, there are a couple meaningful TODOs left, and I haven't written the tests yet -- but the most important stuff is here.

@BramGruneir, would you be willing to review this? Or should I find someone else?

[cockroach-teamcity]

This change is 

[BramGruneir]

I'll take a look.

---

Comments from Reviewable

[petermattis]

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

---

pkg/config/zone.proto, line 66 at r1 (raw file):

  // the NumReplicas replicas. Only required constraints are currently allowed.
  // As of v2.0, this field cannot be set if the Constraints field is.
  repeated Constraints replicas = 7 [(gogoproto.nullable) = false];

This isn't taking the Constraint.num_replicas proposed in #19985. Did you run into difficulties with that approach?

---

pkg/config/zone.proto, line 84 at r1 (raw file):

  // order in which the constraints are stored is arbitrary and may change.
  // https://github.com/cockroachdb/cockroach/blob/master/docs/RFCS/20160706_expressive_zone_config.md#constraint-system
  // As of v2.0, this field cannot be set if the Replicas field is.

Does this comment belong here? There is no Replicas field in ZoneConfig.

---

Comments from Reviewable

[a-robinson]

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

---

pkg/config/zone.proto, line 66 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This isn't taking the Constraint.num_replicas proposed in #19985. Did you run into difficulties with that approach?

I tried this approach because I thought it would be simpler all-around, but ended up finding that something like the num_replicas approach will be easier to handle correctly. I just haven't changed it over yet -- there are a couple TODOs about this in allocator_scorer.go.

---

pkg/config/zone.proto, line 84 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Does this comment belong here? There is no Replicas field in ZoneConfig.

Does not belong here. I'll move it in my next round of cleanups.

---

Comments from Reviewable

[BramGruneir]

I like the analyzedConstraints struct and that part of the scorer seems quite clear. I'm getting a little lost in the rebalanceCandidates part. Even with the numbered sections. A lot is going on there and trying to see how the different scenarios play out is a little tough.

---

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

pkg/config/zone.proto, line 44 at r1 (raw file):

    // POSITIVE will attempt to ensure all stores the replicas are on has this
    // constraint.
    POSITIVE = 0;

Can we deprecate positive yet?

---

pkg/config/zone.proto, line 66 at r1 (raw file):

  // the NumReplicas replicas. Only required constraints are currently allowed.
  // As of v2.0, this field cannot be set if the Constraints field is.
  repeated Constraints replicas = 7 [(gogoproto.nullable) = false];

So how will this work if this is > the number of replicas? Only the first [:n] match?

---

pkg/storage/allocator_scorer.go, line 909 at r1 (raw file):

					}
					if !storeHasConstraint(store, constraint) {
						valid = false

Instead of using the valid variable, perhaps just use a loop label and continue to the next outer loop? Might be more readable.

---

pkg/storage/allocator_scorer.go, line 945 at r1 (raw file):

			}
			if !storeHasConstraint(store, constraint) {
				valid = false

Instead of using the valid variable, perhaps just use a loop label and continue to the next outer loop? Might be more readable.

---

Comments from Reviewable

[a-robinson]

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

pkg/config/zone.proto, line 66 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

I tried this approach because I thought it would be simpler all-around, but ended up finding that something like the num_replicas approach will be easier to handle correctly. I just haven't changed it over yet -- there are a couple TODOs about this in allocator_scorer.go.

Although, to be clear, it has to be Constraints.num_replicas, not Constraint.num_replicas. We have to be able to allow setting the number of replicas for a set of value/key-value constraints, not for just a single value or key-value (or else trying to specify that you want one replica in continent=us,region=us-east and one in continent=us,region=us-west would be very awkward).

---

Comments from Reviewable

[petermattis]

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

pkg/config/zone.proto, line 66 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Although, to be clear, it has to be Constraints.num_replicas, not Constraint.num_replicas. We have to be able to allow setting the number of replicas for a set of value/key-value constraints, not for just a single value or key-value (or else trying to specify that you want one replica in continent=us,region=us-east and one in continent=us,region=us-west would be very awkward).

Got it. Will you also have to change ZoneConfig.constraints to be a repeated field?

---

Comments from Reviewable

[a-robinson]

Thanks for the feedback, I'll clean that long block of code up and knock out some TODOs today. No need to look at the code again until I re-ping the PR.

---

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

pkg/config/zone.proto, line 44 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Can we deprecate positive yet?

I'd be fine with doing so since it was never truly supported in the first place. No need to add more to this PR, though. I'll do that separately.

---

pkg/config/zone.proto, line 66 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

So how will this work if this is > the number of replicas? Only the first [:n] match?

We should validate zone configs to not allow that. As written, the new code will just satisfy as many of them as it can, not favoring any in particular.

---

pkg/config/zone.proto, line 66 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Got it. Will you also have to change ZoneConfig.constraints to be a repeated field?

Yup, that's the plan.

---

pkg/storage/allocator_scorer.go, line 909 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Instead of using the valid variable, perhaps just use a loop label and continue to the next outer loop? Might be more readable.

The amount of nesting here is certainly ugly, but I find resorting to loop labels to be less readable. Maybe it's just me? Do we prefer using labels elsewhere?

---

pkg/storage/allocator_scorer.go, line 945 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Instead of using the valid variable, perhaps just use a loop label and continue to the next outer loop? Might be more readable.

Ditto

---

Comments from Reviewable

[a-robinson]

Alright, mostly done tweaking. Only a couple very small tweaks left.

---

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

---

pkg/config/zone.proto, line 66 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Yup, that's the plan.

Done.

---

pkg/config/zone.proto, line 84 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Does not belong here. I'll move it in my next round of cleanups.

Done.

---

Comments from Reviewable

[a-robinson]

As of the start of the week, there's really just one thing left to do here -- integrate store/node attributes in alongside localities -- and I probably won't be getting to it tonight or tomorrow. Mind reviewing everything other than that in the next day or two?

I also need to would like to add even more unit tests, but that's easier to justify squeezing in on Friday.

---

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

---

Comments from Reviewable

[BramGruneir]

Ok, did a pass through everything. But I'm going to have to spend some serious time thinking through some of the situations.

I'll try to do that tonight.

---

Reviewed 4 of 4 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, 7 of 8 files at r5, 8 of 8 files at r6, 4 of 4 files at r7, 4 of 4 files at r8.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

pkg/config/zone.proto, line 66 at r5 (raw file):

  // As of v2.0, only REQUIRED constraints are allowed when num_replicas is
  // set to a non-zero value.
  optional int32 num_replicas = 1 [(gogoproto.nullable) = false];

Is this reusing number 1? Shouldn't it be 7?

---

pkg/storage/allocator_scorer.go, line 909 at r1 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

The amount of nesting here is certainly ugly, but I find resorting to loop labels to be less readable. Maybe it's just me? Do we prefer using labels elsewhere?

I've used a mix of both. Depending on readability. Up to you. I find not having to track an extra variable useful.

---

pkg/storage/allocator_scorer.go, line 188 at r4 (raw file):

// compare is analogous to strcmp in C or string::compare in C++ -- it returns
// a positive result if c is a better fit for the range than o, 0 if they're
// equivalent, or a positive result if o is a better fit than c. The magnitude

positive for both?

---

pkg/storage/allocator_scorer.go, line 1079 at r5 (raw file):

		case constraint.Type == config.Constraint_PROHIBITED && hasConstraint:
			return false, 0
		case (constraint.Type == config.Constraint_POSITIVE && hasConstraint):

This positive is going away right? Which changes this function entirely.

And it was fixed in the next commit. Nevermind.

---

pkg/storage/allocator_scorer.go, line 1049 at r8 (raw file):

	}

	// Check the store against all the constraints. If it matches a constraint at

Maybe move this to be the description for the function itself.

---

pkg/storage/allocator_scorer_test.go, line 438 at r8 (raw file):

		{
			name: "required constraint",
			constraints: []config.Constraints{

The naming of this is just terrible now.

constraints -> []Constraints -> Constraints -> []Constraint then Type Constraint_X....

Ugh.

---

Comments from Reviewable

[BramGruneir]

Ok, overall approach seems sound to me. Code looks good. But it's going to need a good amount of testing. And I'm worried about how easily we will be able to back it out if things start going wrong.

What's your approach to this going to be?

Also, this is a huge PR, probably a good idea to get someone else's eyes on it just in case.

---

Comments from Reviewable

[a-robinson]

Thanks for the reviews!

Now that I have some free time again, my plan is to finish handling the node/store attributes than write some higher-level unit tests (that test at the AllocateTarget/RemoveTarget/RebalanceTarget level). I don't expect many issues to come up, though -- in my opinion the most likely places to hit problems were in the constraint checking logic themselves and in the new grouping of rebalance targets by rebalance diversity score (i.e. in the fix to #20751), both of which I've already added tests / test cases for.

---

Review status: 0 of 9 files reviewed at latest revision, 8 unresolved discussions.

---

pkg/config/zone.proto, line 66 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Is this reusing number 1? Shouldn't it be 7?

Honestly I'm not quite sure why this is already on field 6. I'll switch the new one to 7.

---

pkg/storage/allocator_scorer.go, line 909 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

I've used a mix of both. Depending on readability. Up to you. I find not having to track an extra variable useful.

It's not relevant anymore anyways :)

---

pkg/storage/allocator_scorer.go, line 188 at r4 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

positive for both?

Fixed, thanks.

---

pkg/storage/allocator_scorer.go, line 1079 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

This positive is going away right? Which changes this function entirely.

And it was fixed in the next commit. Nevermind.

Yeah, sorry!

---

pkg/storage/allocator_scorer.go, line 1049 at r8 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Maybe move this to be the description for the function itself.

Done.

---

pkg/storage/allocator_scorer_test.go, line 438 at r8 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

The naming of this is just terrible now.

constraints -> []Constraints -> Constraints -> []Constraint then Type Constraint_X....

Ugh.

Yeah, it's certainly not good. I'm not sure what to change at this point, though.

---

Comments from Reviewable

[a-robinson]

Alright, I've finished off the final TODO about handling node/store attributes. I'll add some additional tests tonight before my flight, but I'm already about half asleep, so no promises about how many.

---

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

---

Comments from Reviewable

[petermattis]

Sure, I'll give this a thorough review later today (or tonight).

[petermattis]

Apologies for the delay. There were a lot more changes here than I was expecting. Most of my comments are minor. The major question which I wasn't able to answer: if you don't specify a non-zero Constraints.num_replicas, are the heuristics identical to before. If no, what has changed and where can I see the effect on test output. If the rebalancing decisions are identical in the absense of Constraints.num_replicas, then .

As noted below, some of the allocator functions have become beasts. Similarly, some of the testing, while rigorous, is hard to verify for correctness. I think utilizing testutils/datadriven can help here, but will initially require a bit more investment as we'll need to be able to parse the constraints and some specification of stores. I'm sure given the current test complexity that this investment will be worthwhile. I don't think we need to do this work before merging this PR, but I think it is worth doing very soon after.

Lastly, the additional logic has me worried about the performance of this code. We've generally not paid much attention to allocation performance, but as clusters grow it will become an increasing concern.

---

Review status: 0 of 10 files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.

---

pkg/config/zone.proto, line 89 at r17 (raw file):

  // NOTE: The sum of the num_replicas fields of the Constraints must add up to
  // ZoneConfig.num_replicas, or there must be no more than a single Constraints
  // field with num_replicas set to 0.

Why do we require the sum of the num_replicas fields to add up to ZoneConfig.num_replicas. It seems sensible to say that 2 replicas must exist in +region=eu while the 3rd replica can live anywhere. The way I was imagining this would be implemented I don't see a need for this requirement.

---

pkg/roachpb/metadata.go, line 63 at r17 (raw file):

// Equals returns whether the Attributes lists are equivalent, meaning that
// they contain the same attributes regardless of ordering.

Perhaps mention in this sentence at the attribute lists are treated as sets and duplicates are ignored. That is, [a, a, b] == [a, b, b].

---

pkg/storage/allocator_scorer.go, line 517 at r17 (raw file):

// most qualified. The second list is of all potential stores that could be
// used as rebalancing receivers, ordered from best to worst.
func rebalanceCandidates(

This method has certainly bloated over time. We need to take a look at breaking it into more digestible/testable pieces, though not for this PR.

---

pkg/storage/allocator_scorer.go, line 519 at r17 (raw file):

func rebalanceCandidates(
	ctx context.Context,
	fullSL StoreList,

The term full here could also be interpreted as "full" stores. Perhaps s/fullSL/allStores/g

---

pkg/storage/allocator_scorer.go, line 569 at r17 (raw file):

	// 2. For each store, determine the stores that would be the best replacements
	//		on the basis of constraints, disk fullness, and diversity. Only the best

Nit: unusual indentation on this line.

---

pkg/storage/allocator_scorer.go, line 654 at r17 (raw file):

	if !needRebalance {
		for _, existing := range existingStores {
			var comparableSL StoreList

comparableSL vs comparableStores is quite confusing.

---

pkg/storage/allocator_scorer.go, line 678 at r17 (raw file):

	// 4. Create sets of rebalance options, i.e. groups of candidate stores and
	//		the existing replicas that they could legally replace in the range.  We
	//		have to make a separate set of these for each group of comparableStores.

Nit: excessive indentation on these lines.

---

pkg/storage/allocator_scorer.go, line 968 at r17 (raw file):

	// constraints the store satisfies. This field is unused if there are no
	// constraints.
	satisfies map[roachpb.StoreID][]int

Not necessary for this PR, but we should really be caching this info. The set of constraints and stores in a cluster change slowly, there isn't any need to be calculating this on every allocation decision.

---

pkg/storage/allocator_scorer.go, line 1006 at r17 (raw file):

// all replicas. If this returns false, then no replica could ever be
// considered "necessary" for the range.
func needSpecificReplicas(analyzed analyzedConstraints) bool {

This function looks unused.

---

pkg/storage/allocator_scorer.go, line 1044 at r17 (raw file):

}

func removeConstraintsCheck(

This function deserves a doc comment as well. I'm also a bit confused about the naming. What is being "removed"?

---

pkg/storage/allocator_scorer_test.go, line 608 at r17 (raw file):

			},
			expectedNecessary: map[roachpb.StoreID]bool{},
		},

These tests cases are difficult to parse and I imagine they are difficult to write. It would be better to use the testutils/datadriven framework. I don't think this comment should hold up this PR, but I think we should follow up on on it soon.

---

Comments from Reviewable

[a-robinson]

if you don't specify a non-zero Constraints.num_replicas, are the heuristics identical to before

Not in all cases. See below.

If no, what has changed and where can I see the effect on test output.

Fixing #20751 is a change in behavior in cases where some localities are more full than others for reasons outside the system's control -- e.g. if one has more nodes than the others or if some constraints have required more ranges to be in one. In such cases, this does a better job of balancing ranges amongst the nodes that are more or less full because we compare them amongst each other now rather than comparing them to the StoreList averages of all the stores in the cluster. None of the existing tests really cover this, though. I've manually verified that this makes for much better balance on the reproduction from #20241 and have had it on my list of immediate follow-up work to add similar unit tests, but it'd be fair to require them before merging this.

Fixing #20751 is the only change of existing behavior, though.

I think utilizing testutils/datadriven can help here

Well that's nifty looking. To be honest, I don't understand at first glance what about it will make the tests currently there much clearer, but can see that it'd help with expanding them to cover more sets of stores and localities.

Lastly, the additional logic has me worried about the performance of this code. We've generally not paid much attention to allocation performance, but as clusters grow it will become an increasing concern.

Yeah, that's one of my concerns as well. I tried to stay conscious of allocations, but had to back out some of my premature optimizations that broke due to the need to handle store/node attributes. I still haven't seen this stuff show up in benchmarks, but I haven't looked for it on a cluster that was using localities and constraints in a meaningful way that was larger than 8 nodes. It didn't show up as meaningful in an 8-node repro of #20751 with about 1300 ranges, for example.

Caching things between runs would certainly be a win, but it'll be enough of a step up in code complexity that I don't at all regret doing a more direct version first. We can make things more modular in the process, with everything pulled out of the storage package once refactorings are allowed again.

---

Review status: 0 of 10 files reviewed at latest revision, 19 unresolved discussions.

---

pkg/config/zone.proto, line 89 at r17 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Why do we require the sum of the num_replicas fields to add up to ZoneConfig.num_replicas. It seems sensible to say that 2 replicas must exist in +region=eu while the 3rd replica can live anywhere. The way I was imagining this would be implemented I don't see a need for this requirement.

I imposed that to simplify things for myself, but I think at this point it would work fine if we added some extra state to analyzedConstraint to make clear when ZoneConfig.NumReplicas > sum(Constraints.NumReplicas), and in such cases treating replicas that don't match any of the constraints as valid=true necessary=false. It'd require more testing, of course.

That also opens the door to allowing constraints that apply to the entire range alongside per-replica constraints, which feel kind of awkward to specify using the format in #22819, but maybe that's just me.

Also, to be honest, I may be misinterpreting it but I don't believe the implementation as originally proposed in #19985 (comment) works for removing/rebalancing, just for adding replicas.

---

pkg/roachpb/metadata.go, line 63 at r17 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Perhaps mention in this sentence at the attribute lists are treated as sets and duplicates are ignored. That is, [a, a, b] == [a, b, b].

Done.

---

pkg/storage/allocator_scorer.go, line 517 at r17 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This method has certainly bloated over time. We need to take a look at breaking it into more digestible/testable pieces, though not for this PR.

Ack.

---

pkg/storage/allocator_scorer.go, line 519 at r17 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The term full here could also be interpreted as "full" stores. Perhaps s/fullSL/allStores/g

Done.

---

pkg/storage/allocator_scorer.go, line 569 at r17 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: unusual indentation on this line.

Done.

---

pkg/storage/allocator_scorer.go, line 654 at r17 (raw file):

Previously, petermattis (Peter Mattis) wrote…

comparableSL vs comparableStores is quite confusing.

Yeah. I've changed it to just sl which actually seems more clear in the context, and added an explanatory comment to the block of code. Let me know if you don't think that's enough.

---

pkg/storage/allocator_scorer.go, line 678 at r17 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Nit: excessive indentation on these lines.

Done.

---

pkg/storage/allocator_scorer.go, line 1006 at r17 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This function looks unused.

Indeed, thanks.

---

pkg/storage/allocator_scorer.go, line 1044 at r17 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This function deserves a doc comment as well. I'm also a bit confused about the naming. What is being "removed"?

store is an existing replica. that's being considered for removal by removeCandidate. It's analogous to diversityRemovalScore. If there are different names that would be more clear for these, it'd be nice. As is, I've gone for trying to name associated methods similarly.

The function is determining whether store is valid and whether it's necessary from a constraint satisfaction perspective. This is to ensure that we prefer removing an invalid replica before a valid one, and an unnecessary replica (one which doesn't satisfy any constraints that aren't also satisfied by another replica) before a necessary replica.

Sorry about not adding comments to these methods, they aren't obvious.

---

pkg/storage/allocator_scorer_test.go, line 608 at r17 (raw file):

Previously, petermattis (Peter Mattis) wrote…

These tests cases are difficult to parse and I imagine they are difficult to write. It would be better to use the testutils/datadriven framework. I don't think this comment should hold up this PR, but I think we should follow up on on it soon.

Writing them was easy while the state of which stores have which localities/attributes was in my head, but I understand that interpreting them will pretty much require loading that state back in. Luckily these tests are almost totally deterministic, so you should only need to page them in when seriously modifying the allocator logic.

---

Comments from Reviewable

[petermattis]

Fixing #20751 is the only change of existing behavior, though.

Ack. Thanks for the explanation.

I think utilizing testutils/datadriven can help here

Well that's nifty looking. To be honest, I don't understand at first glance what about it will make the tests currently there much clearer, but can see that it'd help with expanding them to cover more sets of stores and localities.

The current tests require keeping a lot of state in your head, understanding the localities and attributes and what not. Data driven tests would also need some initialization like that, but I think we could a) make that clearer and b) have different files covering different setups. I'm 90% confident it will expand the test coverage and make this code better and more maintainable.

Caching things between runs would certainly be a win, but it'll be enough of a step up in code complexity that I don't at all regret doing a more direct version first. We can make things more modular in the process, with everything pulled out of the storage package once refactorings are allowed again.

Agreed on the step up in complexity and the decision to not pile that work into this PR. My concern about allocator performance is an observation that we'll probably have to address soon, but not today.

---

Review status: 0 of 10 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

pkg/config/zone.proto, line 89 at r17 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

I imposed that to simplify things for myself, but I think at this point it would work fine if we added some extra state to analyzedConstraint to make clear when ZoneConfig.NumReplicas > sum(Constraints.NumReplicas), and in such cases treating replicas that don't match any of the constraints as valid=true necessary=false. It'd require more testing, of course.

That also opens the door to allowing constraints that apply to the entire range alongside per-replica constraints, which feel kind of awkward to specify using the format in #22819, but maybe that's just me.

Also, to be honest, I may be misinterpreting it but I don't believe the implementation as originally proposed in #19985 (comment) works for removing/rebalancing, just for adding replicas.

Ack. Seems reasonable for now and we can relax this restriction later.

---

pkg/storage/allocator_scorer_test.go, line 608 at r17 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Writing them was easy while the state of which stores have which localities/attributes was in my head, but I understand that interpreting them will pretty much require loading that state back in. Luckily these tests are almost totally deterministic, so you should only need to page them in when seriously modifying the allocator logic.

The major benefit of testutils/datadriven is that when the allocator logic changes, you can run the test with --rewrite in order to update the test files. Then you can diff the changes and verify that they are as expected. datadriven tests are sometimes slightly more verbose in their output, but easier for a reader to understand.

---

Comments from Reviewable

[a-robinson]

Merged as part of #22819