kvserver: Replace multiTestContext with TestCluster in client_replica_gc_test

[lunevalex]

Makes progress on #8299

multiTestContext is legacy construct that is deprecated in favor of running
tests via TestCluster. This is one PR out of many to remove the usage of
multiTestContext in the client_replica_gc test cases.

Release note: none

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lunevalex and @tbg)

---

pkg/kv/kvserver/client_replica_gc_test.go, line 38 at r1 (raw file):

	const numStores = 3

	// Use actual engines (not in memory) because the in-mem ones don't write

I'm surprised we need this. I don't quite understand the justification. You say the test would still pass but then say it would probably fail. What were you seeing without it? The in-memory engine should still support a mock file system that every disk access should go through. We seem to handle this correctly below with calls like `fs.WriteFile(eng, ...). Are we forgetting to do that somewhere?

---

pkg/kv/kvserver/replica_gc_queue.go, line 92 at r1 (raw file):

	metrics                           ReplicaGCQueueMetrics
	db                                *kv.DB
	replicaGCQueueInactivityThreshold time.Duration

Are we actually using this? I would have expected to see something in replicaGCShouldQueueImpl.

---

pkg/kv/kvserver/replica_gc_queue.go, line 98 at r1 (raw file):

func newReplicaGCQueue(store *Store, db *kv.DB, gossip *gossip.Gossip) *replicaGCQueue {
	replicaGCQueueInactivityThreshold := ReplicaGCQueueInactivityThreshold
	if store.TestingKnobs().ReplicaGCQueueInactivityThreshold != 0 {

nit: this kind of thing is often done using a temp variable like:

if knob := store.TestingKnobs().ReplicaGCQueueInactivityThreshold; knob != 0 {
	replicaGCQueueInactivityThreshold = knob
}

[lunevalex]

---

pkg/kv/kvserver/replica_gc_queue.go, line 92 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Are we actually using this? I would have expected to see something in replicaGCShouldQueueImpl.

Good catch, I did not finish plumbing it through but the test passed. This was to replace the bit where were were manually moving the clock. It looks like its not needed because we disable the queue at the start of the cluster and then enable it, which means this gets picked up on the first run. Going to remove this.

[lunevalex]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lunevalex, @nvanbenschoten, and @tbg)

---

pkg/kv/kvserver/client_replica_gc_test.go, line 38 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'm surprised we need this. I don't quite understand the justification. You say the test would still pass but then say it would probably fail. What were you seeing without it? The in-memory engine should still support a mock file system that every disk access should go through. We seem to handle this correctly below with calls like `fs.WriteFile(eng, ...). Are we forgetting to do that somewhere?

Great question, I did not change the logic or conditions of the test just it's usage of multiTestContext. I will test it out to see it makes a difference.

[lunevalex]

---

pkg/kv/kvserver/client_replica_gc_test.go, line 38 at r1 (raw file):

Previously, lunevalex wrote…

Great question, I did not change the logic or conditions of the test just it's usage of multiTestContext. I will test it out to see it makes a difference.

Actually you are right I am not sure why I thought the original test was creating file system based engines, I removed all of that and switched to in-memory ones and everything works.

[nvanbenschoten]

thanks for the update, the PR is nice and straightforward now.

Reviewed 3 of 3 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

[lunevalex]

bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)