kv: Don't automatically rerun replicate_queue

[andrewbaptist]

Previously after the replicate queue completed processing a
change, it would immediately see if there were other changes
that could be done to a range. This bypassed the normal
priority order and meant that a replica might process a low
priority, expensive job before a high priority one. The intented
behavior is to check if there is additional work for the replica
and requeue at the correct priority.

Release note: None

[cockroach-teamcity]

This change is 

[andrewbaptist]

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

---

pkg/kv/kvserver/replicate_queue.go line 550 at r1 (raw file):

Previously, kvoli (Austen) wrote…

I think this should go within the if statement below.

Done

[andrewbaptist]

This changes the replicate_queue does not automatically run the next action, but instead queues it up to run at the appropriate priority.

Doing this created 2 unit test failures, that I dealt with differently, but I wanted to see if anyone had concerns on this:

TestLeasePreferencesDuringOutage - In this case there was an expectation that after a voter was added that we would check lease preferences immediately and move the lease to the correct node if possible. This seems like a reasonable expectation, but the way it was getting there was a little convoluted. It expected a AllocatorReplaceDeadVoter call followed immediately by a AllocatorConsiderRebalance call to move the lease. With the new structure, the second call would be low priority and in a real system what would happen is that all the dead voters would be replaced before any of the rebalances happen. Adding a lease preference check after the add voter call fixed this.

TestReplicateQueueSeesLearnerOrJointConfig - In this case, the test is creating a situation where voters are partial complete, and it expects the replicate queue to “fix everything”. I don’t understand all the transitions in this test, especially the second one where the replicate queue has to call AllocatorFinalizeAtomicReplicationChange twice. I didn’t change the behavior of this test other than requiring enqueue to be called multiple times, and it might be because of the withStopAfterLearnerAtomic parameter being why the AddVoter is not successful the first time.

[kvoli]

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

---

-- commits line 6 at r2:
nit: done to a range, rather than replica.

---

-- commits line 7 at r2:
nit: replic -> replica.

---

pkg/kv/kvserver/replicate_queue.go line 925 at r2 (raw file):

		// We require the lease in order to process replicas, so
		// repl.store.StoreID() corresponds to the lease-holder's store ID.
		_, err := rq.shedLease(

If we did shed the lease here, then there shouldn't be a requeue for this replica because it won't have a lease. Can we set !lhBeingRemoved=true if rq.ShedLease returns true.

The replica in the queue will try and acquire the lease and (likely) fail the raft leader check its processed again, we can avoid the added processing.

---

pkg/kv/kvserver/replica_learner_test.go line 751 at r2 (raw file):

	tc := testcluster.StartTestCluster(t, 3, base.TestClusterArgs{
		ServerArgs:      base.TestServerArgs{Knobs: knobs},
		ReplicationMode: base.ReplicationManual,

since it’s in replication manual mode, the requeues in the process loop are being ignored. So it won't be testing your patch completely here.

cockroach/pkg/kv/kvserver/queue.go

Lines 639 to 641 in 9b5a2b7

	bq.mu.Lock()
	stopped := bq.mu.stopped || bq.mu.disabled
	bq.mu.Unlock()

---

pkg/kv/kvserver/replica_learner_test.go line 773 at r2 (raw file):

		}
		for _, message := range expectedMessages {
			trace, processErr, err := store.Enqueue(

See below on manually requeuing.

---

pkg/kv/kvserver/replica_learner_test.go line 797 at r2 (raw file):

			allocatorimpl.AllocatorFinalizeAtomicReplicationChange,
			allocatorimpl.AllocatorAddVoter,
			allocatorimpl.AllocatorFinalizeAtomicReplicationChange,

This is in the scope for the second stop, ltk.withStopAfterJointConfig, it will never finalize the atomic change itself until another replicate queue does it. So there are two AllocatorFinalizeAtomicReplicationChanges here.

---

pkg/kv/kvserver/replica_learner_test.go line 801 at r2 (raw file):

		}
		for _, message := range expectedMessages {
			trace, processErr, err := store.Enqueue(

It would be good to test the requeuing functionality does what is expected for this test, rather than manually requeuing.

You could try disabling the replica scanner:

cockroach/pkg/kv/kvserver/allocator/allocatorimpl/allocator.go

Line 583 in 9b5a2b7

Then after ltk.withStopAfterLearnerAtomic you then enable the replicate queue.

Then assert on the action in processOneChange:

cockroach/pkg/kv/kvserver/replicate_queue.go

Line 602 in 9b5a2b7

log.VEventf(ctx, 1, "next replica action: %s", action)

Likewise for the other stop.

[andrewbaptist]

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

---

pkg/kv/kvserver/replicate_queue.go line 925 at r2 (raw file):

Previously, kvoli (Austen) wrote…

If we did shed the lease here, then there shouldn't be a requeue for this replica because it won't have a lease. Can we set !lhBeingRemoved=true if rq.ShedLease returns true.

The replica in the queue will try and acquire the lease and (likely) fail the raft leader check its processed again, we can avoid the added processing.

OK, good call on this.

---

pkg/kv/kvserver/replica_learner_test.go line 751 at r2 (raw file):

Previously, kvoli (Austen) wrote…

since it’s in replication manual mode, the requeues in the process loop are being ignored. So it won't be testing your patch completely here.

cockroach/pkg/kv/kvserver/queue.go

Lines 639 to 641 in 9b5a2b7

	bq.mu.Lock()
	stopped := bq.mu.stopped || bq.mu.disabled
	bq.mu.Unlock()

OK, changed the test

---

pkg/kv/kvserver/replica_learner_test.go line 773 at r2 (raw file):

Previously, kvoli (Austen) wrote…

See below on manually requeuing.

OK, done

---

pkg/kv/kvserver/replica_learner_test.go line 797 at r2 (raw file):

Previously, kvoli (Austen) wrote…

This is in the scope for the second stop, ltk.withStopAfterJointConfig, it will never finalize the atomic change itself until another replicate queue does it. So there are two AllocatorFinalizeAtomicReplicationChanges here.

OK, makes sense

---

pkg/kv/kvserver/replica_learner_test.go line 801 at r2 (raw file):

Previously, kvoli (Austen) wrote…

It would be good to test the requeuing functionality does what is expected for this test, rather than manually requeuing.

You could try disabling the replica scanner:

cockroach/pkg/kv/kvserver/allocator/allocatorimpl/allocator.go

Line 583 in 9b5a2b7

Then after ltk.withStopAfterLearnerAtomic you then enable the replicate queue.

Then assert on the action in processOneChange:

cockroach/pkg/kv/kvserver/replicate_queue.go

Line 602 in 9b5a2b7

log.VEventf(ctx, 1, "next replica action: %s", action)

Likewise for the other stop.

Done, this idea worked well

[kvoli]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks, @andrewbaptist, and @kvoli)

---

pkg/kv/kvserver/allocator/allocatorimpl/allocator.go line 628 at r3 (raw file):

	}

	return a.computeAction(ctx, conf, desc.Replicas().VoterDescriptors(),

Why did you drop the internal method computeAction here?

[andrewbaptist]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks and @kvoli)

---

pkg/kv/kvserver/allocator/allocatorimpl/allocator.go line 628 at r3 (raw file):

Previously, kvoli (Austen) wrote…

Why did you drop the internal method computeAction here?

The method was only called in one case by the method right above it and artificially split between what was checked in the first vs the second. I should probably not have included this change in this PR though (or maybe as a separate commit) as it is unrelated to some of the other changes. It just threw me off first when I was looking at ComputeAction vs computeAction and I was trying to understand why we needed some checks in the first method and some in the second. When I realized there was no technical reason for the split, it seemed cleaner.

[kvoli]

This change seems significant enough to warrant some small roachprod testing to see the effects "overall" and make sure everything works as expected.

You mentioned @AlexTalks has a good test. You could run the largest existing roachtest (compare to a control).

./build/builder.sh mkrelease
dev build roachtest
roachtest run decommissionBench/nodes=8/cpu=16/warehouses=3000
roachprod list
# find your cluster to get the $cluster_name - it will be your username-xxxx
roachprod adminui $cluster_name
# monitor the test.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks, @andrewbaptist, and @kvoli)

---

pkg/kv/kvserver/allocator/allocatorimpl/allocator.go line 628 at r3 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

The method was only called in one case by the method right above it and artificially split between what was checked in the first vs the second. I should probably not have included this change in this PR though (or maybe as a separate commit) as it is unrelated to some of the other changes. It just threw me off first when I was looking at ComputeAction vs computeAction and I was trying to understand why we needed some checks in the first method and some in the second. When I realized there was no technical reason for the split, it seemed cleaner.

Given this pattern is used for most of the allocator methods, where there is a mostly empty exported method calling an unexported method - It would be better to leave it out of this patch, then reconsider all of them at once rather than adhoc.

[AlexTalks]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks, @andrewbaptist, and @kvoli)

---

pkg/kv/kvserver/replicate_queue.go line 925 at r2 (raw file):

Previously, andrewbaptist (Andrew Baptist) wrote…

OK, good call on this.

Discussed in person - want to take a look to see if this is the correct place to do this (or what else needs to/should change).

[AlexTalks]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks, @andrewbaptist, and @kvoli)

---

pkg/kv/kvserver/replicate_queue.go line 532 at r4 (raw file):

			// few seconds. By retrying immediately we will choose the "second best"
			// node to send to.
			// TODO: This is probably suboptimal behavior. In the case where there is

nit: TODO(andrewbaptist) (or, for that matter, put TODO(sarkesian) if needed).

I think the linter would catch this in any case.

[andrewbaptist]

Tested with Alex, still not "ideal" behavior because stores finish at different times, but better. Alex is analyzing results

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks and @kvoli)

---

pkg/kv/kvserver/replicate_queue.go line 925 at r2 (raw file):

Previously, AlexTalks (Alex Sarkesian) wrote…

Discussed in person - want to take a look to see if this is the correct place to do this (or what else needs to/should change).

OK, The test TestLeasePreferencesDuringOutage fails without this change. Specifically, it is expecting a lease change to happen after we addOrReplaceVoters is called. This test passed before because after we re-ran the replicate queue, we would end up calling considerRebalance which would then rebalance and as a fortunate result also move the lease. This test was a little broken in doing what it is trying to do since if there wasn't a rebalance, this wouldn't have worked. Now the test is a little more accurate. It might make sense long term to always call maybeTransferLeaseAway after any change is made since normally it won't do anything if lease preference is already correct, but that could be overkill.

---

pkg/kv/kvserver/replicate_queue.go line 532 at r4 (raw file):

Previously, AlexTalks (Alex Sarkesian) wrote…

nit: TODO(andrewbaptist) (or, for that matter, put TODO(sarkesian) if needed).

I think the linter would catch this in any case.

Done

---

pkg/kv/kvserver/allocator/allocatorimpl/allocator.go line 628 at r3 (raw file):

Previously, kvoli (Austen) wrote…

Given this pattern is used for most of the allocator methods, where there is a mostly empty exported method calling an unexported method - It would be better to leave it out of this patch, then reconsider all of them at once rather than adhoc.

Done, reverted in this PR