kvserver: fix deadlock on rebalance obj change

[kvoli]

Previously, changing the rebalance objective could lead to inconsistent
locking order between the load based splitter and rebalance objective.
When the objective was updated, the previous method also blocked
batch requests from completing until every replica lb splitter was
reset.

This commit moves the split objective to be a variable owned by the
decider, rather than inferred on each decider operation. The split
objective is updated on a rebalance objective change atomically over
each replica but not atomically over a store. This removes the need for
blocking batch requests until every replica is updated.

Resolves: #97000
Resolves: #97445
Resolves: #97450
Resolves: #97452
Resolves: #97457

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @kvoli)

---

pkg/kv/kvserver/merge_queue.go line 278 at r1 (raw file):

	mergedStats.Add(rhsStats)

	lhsLoadSplitSnap := lhsRepl.loadBasedSplitter.Snapshot(ctx, now.ToTimestamp().GoTime())

Should this be using mq.store.Clock().PhysicalTimestamp()? Does it want the causality tracking provided by the HLC and imbued into now?

---

pkg/kv/kvserver/replica_split_load.go line 223 at r1 (raw file):

	shouldInitSplit := r.loadBasedSplitter.Record(ctx, timeutil.Now(),
		func(obj split.SplitObjective) int {

Now that there are two functions here, consider pulling them out into named local variables to improve the readability of this code.

---

pkg/kv/kvserver/split/decider.go line 56 at r1 (raw file):

}

type LoadSplitConfig interface {

Now that this config is no longer providing the SplitObjective, what is its role? What state is it encapsulating?

---

pkg/kv/kvserver/split/decider.go line 390 at r1 (raw file):

// be in terms of the split objective set by p2; which could be widly wrong.
type LoadSplitSnapshot struct {
	SplitObjective

Is there a benefit to the embedding? If not, consider removing it as it makes the structure look more complexity than it really is (it's just a collection of primitive values).

---

pkg/kv/kvserver/split/decider.go line 396 at r1 (raw file):

// Snapshot returns a consistent snapshot of the decider state.
func (d *Decider) Snapshot(ctx context.Context, now time.Time) LoadSplitSnapshot {

Should all existing users of MaxStat, LastStat, and Threshold be switched over to this method?

[kvoli]

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

---

pkg/kv/kvserver/merge_queue.go line 278 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should this be using mq.store.Clock().PhysicalTimestamp()? Does it want the causality tracking provided by the HLC and imbued into now?

Definitely don't need. It just seemed convenient. Updated.

---

pkg/kv/kvserver/replica_split_load.go line 223 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Now that there are two functions here, consider pulling them out into named local variables to improve the readability of this code.

Good idea. Updated.

---

pkg/kv/kvserver/split/decider.go line 56 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Now that this config is no longer providing the SplitObjective, what is its role? What state is it encapsulating?

It is encapsulating the dynamic values for Retention and Threshold based on the cluster setting underlying it in the production code - or other implementations such as the simulator.

Its role is to avoid the need to push settings gathering down into the decider.

I think an alternative is going back to the original approach, where these are anonymous functions provided at Init. That alternative seems less desirable than what is currently here.

---

pkg/kv/kvserver/split/decider.go line 390 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is there a benefit to the embedding? If not, consider removing it as it makes the structure look more complexity than it really is (it's just a collection of primitive values).

I was going to use it to call format/name. Not worthwhile - removed embedding.

---

pkg/kv/kvserver/split/decider.go line 396 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should all existing users of MaxStat, LastStat, and Threshold be switched over to this method?

Probably - I swapped them.

[nvanbenschoten]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @kvoli)

---

pkg/kv/kvserver/merge_queue.go line 278 at r1 (raw file):

Previously, kvoli (Austen) wrote…

Definitely don't need. It just seemed convenient. Updated.

On this note, I see that we are using timeutil.Now() when calling Decide. Should all uses of the load-based splitter be using (*hlc.Clock).PhysicalTime to ensure a consistent notion of time?

---

pkg/kv/kvserver/split/decider.go line 56 at r1 (raw file):

Previously, kvoli (Austen) wrote…

It is encapsulating the dynamic values for Retention and Threshold based on the cluster setting underlying it in the production code - or other implementations such as the simulator.

Its role is to avoid the need to push settings gathering down into the decider.

I think an alternative is going back to the original approach, where these are anonymous functions provided at Init. That alternative seems less desirable than what is currently here.

I see, that makes sense. Thanks for explaining.

By the way, does this exported type need a comment? Is CI complaining?

[kvoli]

TYFTR

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

---

pkg/kv/kvserver/merge_queue.go line 278 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

On this note, I see that we are using timeutil.Now() when calling Decide. Should all uses of the load-based splitter be using (*hlc.Clock).PhysicalTime to ensure a consistent notion of time?

We should. I've updated every call to use PhysicalTime().

---

pkg/kv/kvserver/split/decider.go line 56 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I see, that makes sense. Thanks for explaining.

By the way, does this exported type need a comment? Is CI complaining?

Not any more - not sure why. I personally always found the comments on methods more useful than the iface.

[kvoli]

bors r=nvanbenschoten

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[kvoli]

Odd test to fail on CI. Looks flaky but I can't repro locally.

./dev test pkg/ccl/schemachangerccl -v -f  TestBackup_base_alter_table_add_check_unvalidated/backup/restore_stage_0_of_0 --show-logs --stress
 ...
3138 runs so far, 0 failures, over 1m40s

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)