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

pkg/kv/kvserver/store.go, line 807 at r1 (raw file):

// use the TestCluster dont have an easy way to get at the config.
// TODO(lunevalex): Remove the LeaseExpiration on StoreConfig, once multiTestContext is removed.
func (s *Store) LeaseExpiration() int64 {

I would expose the StoreConfig instead.

pkg/server/testserver.go, line 1367 at r1 (raw file):

// an expiration based lease.
//
// Calling this multiple times is undefined (but see

This should work fine ,does it not? I understand you're just cargo culting but let's take a moment to find out why the old code is so weird.

I think you wanted to mentionTestCluster.ScratchRange in the comment.

pkg/testutils/testcluster/testcluster.go, line 861 at r1 (raw file):

func (tc *TestCluster) ScratchRange(t testing.TB) roachpb.Key {
	if tc.scratchRangeKey != nil {
		return tc.scratchRangeKey

I never quite understood what this caching was for. I guess ScratchRange() is not idempotent, let's find out why and revisit.

pkg/util/hlc/hlc.go, line 148 at r1 (raw file):

// Increment atomically increments the hybrid manual clock's timestamp.
func (m *HybridManualClock) Increment(incr int64) {

Btw nothing prevents this clock from going backwards, which, maybe, is fine? The HLC is going to tick the logicals instead. The real question is which parts of the system don't work with a clock whose walltime does not move forward. Maybe instead of my current approach of calling this situation bad, it is in fact the code that we have in the system that uses hlc timestamps to measure durations that is bad, such as this:

If we wanted to fix this, we'd need to separate out two clock sources: one clock source that powers the hlc (and which may just never move and that's fine), and another one that we use to measure durations. In practice, that just means using timeutil.Now(), etc and never using the hlc timestamps for setting backoffs, i.e. this would just use timeutil.Now():

Of course this messes with how this time source can be mocked in tests, so in the long run it does make more sense to remove timeutil (while continuing to prohibit direct references to time.Now) and instead force use of a more locally scoped clock for this purpose.

Filed #59239

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

pkg/kv/kvserver/store.go, line 807 at r1 (raw file):

OK, was not sure if it was private for a reason. store,go has quite a bit of code that exposes things from the StoreConfig in this manner. I will expose the Config and the do a follow up to clean up the other places where we exposed fields from the config in this manner for testing.

pkg/server/testserver.go, line 1367 at r1 (raw file):

I think it's there because of the split, I tested it locally and calling split on the same key twice works just fine. replica_command has a guard against that If the range starts at the splitKey, we treat the AdminSplit as a no-op and return success instead of throwing an error.. I think given all of this I can clean up the language and get rid of the caching in TestCluster, i.e. the comment below.

pkg/testutils/testcluster/testcluster.go, line 861 at r1 (raw file):

ScratchRange issues a split on the range, so the caching is the guard to protect us from calling the split a second time, which based on the comment above it not really needed.

Reviewed 1 of 5 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @lunevalex, and @tbg)

pkg/testutils/testcluster/testcluster_test.go, line 351 at r2 (raw file):

	require.NotNil(t, lease.Expiration)

	// Verify idempotence of ScratchRangeWithExpirationLease

.