kv: clean up randomized retryable error injection

[nvanbenschoten]

This commit includes a collection of kv-level cleanups for the randomized retryable error injection functionality recently introduced in #107954. Notably, it:

• removes the dependency from kvcoord.TxnCoordSender on kv.Txn by adjusting the parameter of ClientTestingKnobs.TransactionRetryFilter.
• unexports kv.Txn.DebugNameLocked
• deletes COCKROACH_DISABLE_COMMIT_SANITY_CHECK again, which was unused
• removes rng sources in favor of the global rand, for simplicity
• cleans up and adds some comments

Epic: None
Release note: None

[cockroach-teamcity]

This change is 

[arulajmani]

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ecwall, @nvanbenschoten, and @rhu713)

---

pkg/kv/sender.go line 348 at r1 (raw file):

	// TestingShouldRetry returns true if transaction retry errors should be
	// randomly returned to callers. Note, it is the responsibility of

While you're here, s/Note,/Note/g

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 44 at r1 (raw file):

//
// See: https://github.com/cockroachdb/cockroach/pull/73512.
var DisableCommitSanityCheck = envutil.EnvOrDefaultBool("COCKROACH_DISABLE_COMMIT_SANITY_CHECK", false)

Hopefully this is the last time we see this env var 🥲

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 1541 at r1 (raw file):

}

// TestingShouldRetry is part of the TxnSender interface.

Shouldn't this have been caught by some linter? Do you know if something has changed recently?

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 1548 at r1 (raw file):

		return false
	}
	if filter := tc.testingKnobs.TransactionRetryFilter; filter != nil && filter(tc.mu.txn.Clone()) {

nit: is it worth saying we're cloning the transaction here because we don't want the filter to modify the transaction? Or, better yet, should the testing knob be accepting the transaction by value instead to make the contract here clearer?

[nvanbenschoten]

TFTR!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani, @ecwall, and @rhu713)

---

pkg/kv/sender.go line 348 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

While you're here, s/Note,/Note/g

Done.

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 1541 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Shouldn't this have been caught by some linter? Do you know if something has changed recently?

I was also surprised. I'm not aware of any changes but have noticed that I seem to get bugged about this less often these days, so I think something probably has changed.

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go line 1548 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: is it worth saying we're cloning the transaction here because we don't want the filter to modify the transaction? Or, better yet, should the testing knob be accepting the transaction by value instead to make the contract here clearer?

Good idea. Went with the pass-by-value suggestion.

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)