Skip to content

Commit ae68347

Browse files
committed
kvserver: update local store pool directly with AllocationChangeReplicasOp
Previously, `change.Op.ApplyImpact` was used to update the replicateQueue's local store pool state after a successful change replica call to reflect the result of applying the operation. But in practice only `AllocationChangeReplicasOp` used it - other ops update the store pool directly. This commit updates `AllocationChangeReplicasOp` to also update the local store pool directly after a successful `repl.changeReplicasImpl`. In a follow-up, we'll remove `ApplyImpact` from the `AllocationOp` interface entirely, since it adds an unnecessary layer of indirection without providing much value.
1 parent 1e2f0e2 commit ae68347

File tree

1 file changed

+12
-7
lines changed

1 file changed

+12
-7
lines changed

pkg/kv/kvserver/replicate_queue.go

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -816,6 +816,7 @@ func (rq *replicateQueue) applyChange(
816816
replica,
817817
op.Chgs,
818818
replica.Desc(),
819+
op.Usage,
819820
op.AllocatorPriority,
820821
op.Reason,
821822
op.Details,
@@ -914,10 +915,6 @@ func (rq *replicateQueue) processOneChange(
914915
return false, maybeAnnotateDecommissionErr(err, change.Action)
915916
}
916917

917-
// Update the local storepool state to reflect the successful application
918-
// of the change.
919-
change.Op.ApplyImpact(rq.storePool)
920-
921918
// Requeue the replica if it meets the criteria in ShouldRequeue.
922919
return ShouldRequeue(ctx, change, conf), nil
923920
}
@@ -1060,18 +1057,26 @@ func (rq *replicateQueue) changeReplicas(
10601057
repl *Replica,
10611058
chgs kvpb.ReplicationChanges,
10621059
desc *roachpb.RangeDescriptor,
1060+
rangeUsageInfo allocator.RangeUsageInfo,
10631061
allocatorPriority float64,
10641062
reason kvserverpb.RangeLogEventReason,
10651063
details string,
10661064
) error {
10671065
// NB: this calls the impl rather than ChangeReplicas because
10681066
// the latter traps tests that try to call it while the replication
10691067
// queue is active.
1070-
_, err := repl.changeReplicasImpl(
1068+
if _, err := repl.changeReplicasImpl(
10711069
ctx, desc, kvserverpb.SnapshotRequest_REPLICATE_QUEUE, allocatorPriority, reason,
10721070
details, chgs,
1073-
)
1074-
return err
1071+
); err != nil {
1072+
return err
1073+
}
1074+
// On success, update local store pool to reflect the result of applying the
1075+
// operations.
1076+
for _, chg := range chgs {
1077+
rq.storePool.UpdateLocalStoreAfterRebalance(chg.Target.StoreID, rangeUsageInfo, chg.ChangeType)
1078+
}
1079+
return nil
10751080
}
10761081

10771082
func (*replicateQueue) postProcessScheduled(

0 commit comments

Comments
 (0)