kvserver: fix TestStoreRangeSplitAndMergeWithGlobalReads

[andrewbaptist]

Directly set the span config for the range under test rather than setting the ZoneConfig and waiting for it to propagate. In addition to simplifying the test it also makes it run faster.

Fixes: #119230

Epic: none

Release note: None

[cockroach-teamcity]

This change is 

[arulajmani]

Looks great!

Reviewed 1 of 1 files at r1, 3 of 3 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist)

---

-- commits line 2 at r1:
IIUC, we're not reverting the entire commit here, are we? We're only reverting the changes to this particular test?

It seems like that from the diff. Could you clarify this in the title?

---

pkg/spanconfig/testing_knobs.go line 159 at r3 (raw file):

// of DefaultZoneConfigOverride, DefaultSystemZoneConfigOverride,
// OverrideFallbackConf, ConfReaderInterceptor, UseSystemConfigSpanForQueues,
// SpanConfigUpdateInterceptor or SetSpanConfigInterceptor. By using this class

nit: given this is go, let's replace all mentions of class with struct.

---

pkg/spanconfig/testing_knobs.go line 162 at r3 (raw file):

// as an alternative to the others it mainly avoids any need to depend on
// rangefeed timing to propagate the config change.
type WrappingKVSubscriber struct {

Let's move this into the spanconfigtestutils package. Separately, should we rename this to WrappedKVSubscriber?

---

pkg/spanconfig/testing_knobs.go line 163 at r4 (raw file):

// rangefeed timing to propagate the config change.
type WrappingKVSubscriber struct {
	Wrapped KVSubscriber

Let's make these private fields and introduce a constructor to initialize the struct instead.

---

pkg/kv/kvserver/client_split_test.go line 4058 at r5 (raw file):

	ctx := context.Background()
	s := serverutils.StartServerOnly(t, base.TestServerArgs{
		DisableSQLServer: true,

Nice 💯

[andrewbaptist]

Thanks! Also thanks for the feedback on the changes!

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

---

-- commits line 2 at r1:

Previously, arulajmani (Arul Ajmani) wrote…

IIUC, we're not reverting the entire commit here, are we? We're only reverting the changes to this particular test?

It seems like that from the diff. Could you clarify this in the title?

Done

---

pkg/spanconfig/testing_knobs.go line 159 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: given this is go, let's replace all mentions of class with struct.

Done

---

pkg/spanconfig/testing_knobs.go line 162 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Let's move this into the spanconfigtestutils package. Separately, should we rename this to WrappedKVSubscriber?

Moved - I didn't like where it was before, so this is much nicer.

---

pkg/spanconfig/testing_knobs.go line 163 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Let's make these private fields and introduce a constructor to initialize the struct instead.

Changed. I also made AddOverride a method rather than passing in construction. This helps a bit with adding multiple and does not' require configOverride to be public either. I'm not 100% sure its great since it somewhat implies that you can add an override after construction, but other than a comment I'm not sure the best way to do this. Let me know if you think this could be structured differently.

---

pkg/kv/kvserver/client_split_test.go line 4058 at r5 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Nice 💯

Thanks! I'm hoping we can keep rolling this out to more of the tests. It takes 20-25% off the time to run most of the tests so could be a big win in total. I'm tempted to just try and apply it to "all" the files and see which break and then keep any of the ones that don't. Maybe some friday when I have extra time.

[arulajmani]

Reviewed 5 of 5 files at r6, 3 of 3 files at r7, 3 of 3 files at r8, 3 of 3 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andrewbaptist)

---

-- commits line 29 at r8:
nit: test struct WrappedKVSubscriber now

---

pkg/spanconfig/testing_knobs.go line 163 at r4 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

Changed. I also made AddOverride a method rather than passing in construction. This helps a bit with adding multiple and does not' require configOverride to be public either. I'm not 100% sure its great since it somewhat implies that you can add an override after construction, but other than a comment I'm not sure the best way to do this. Let me know if you think this could be structured differently.

I can imagine cases where setting an override (or updating it) after construction could be useful. Previously, we would have to set zone configs and wait for them to reconcile; now, we can just override. I'm pro this change.

---

pkg/kv/kvserver/client_split_test.go line 4058 at r5 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

Thanks! I'm hoping we can keep rolling this out to more of the tests. It takes 20-25% off the time to run most of the tests so could be a big win in total. I'm tempted to just try and apply it to "all" the files and see which break and then keep any of the ones that don't. Maybe some friday when I have extra time.

Yeah, this is neat -- now we have this testing knob to override specific span configs, there's really no need to go through the whole reconciliation process. Maybe one Friday ;)

[andrewbaptist]

TFTR! I fixed the commit message.

bors r=arulajmani

[craig]

Build succeeded:

• local_roachtest_fips
• lint
• docker_image_amd64
• windows_build
• Bazel Essential CI (Cockroach)
• examples_orms
• linux_arm64_build
• linux_amd64_fips_build
• macos_arm64_build
• check_generated_code
• macos_amd64_build
• acceptance
• local_roachtest
• linux_amd64_build