Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

pkg/storage/allocator.go, line 449 at r1 (raw file):

	// stores the replicas are on.
	filteredDescs := make([]roachpb.StoreDescriptor, 0, len(sl.stores))
	for _, s := range sl.stores {

Can't this be simplified to get rid of the exclude variable by using a label? Not sure if we have a policy against labels, but this is an easy case for one.

OUTER:
for _, s := range sl.stores {
  for _, r := range existing {
    if r.NodeID == s.Node.NodeID && r.StoreID != s.StoreID {
      continue OUTER 
    }
  }
  filteredDesc = append(filteredDescs, s)
}

On the other hand, is it worth creating a map to not have to do the inner loop at all? I know it's going to normally be 3 items, but we do this a lot throughout the allocator and balancer, perhaps we should create that map once at the start.

pkg/storage/allocator_test.go, line 1032 at r1 (raw file):

	testCases := []struct {
		existing    []roachpb.ReplicaDescriptor

Since existing is always the same, why keep it in the struct?

pkg/storage/allocator_test.go, line 1042 at r1 (raw file):

	}
	for _, c := range testCases {
		t.Run("", func(t *testing.T) {

Shouldn't the subtest have a name? Just the leaseholder should be fine.

Comments from Reviewable

Review status: 1 of 2 files reviewed at latest revision, 3 unresolved discussions.

pkg/storage/allocator.go, line 449 at r1 (raw file):

Yes, this can be changed as you suggest, though the benefit is small given that this code will be going away when we switch to the new allocator. I'd prefer to leave as-is unless you feel really strongly about it.

pkg/storage/allocator_test.go, line 1032 at r1 (raw file):

Because TestAllocatorTransferLeaseTarget does that. Changed.

pkg/storage/allocator_test.go, line 1042 at r1 (raw file):

We don't have any requirement about giving subtests names. So far, I find doing so onerous and prefer to use the automatically generated names (i.e. #00, #01, etc).

Comments from Reviewable

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

pkg/storage/allocator.go, line 449 at r1 (raw file):

Is this code going away soon? This is part of lease transferring and I haven't ported any lease stuff over yet. I'd like to port if over, but I think there are more important parts of replica rebalancing that need to be addressed first.

That being said, I don't feel strongly about it, I find labels to be almost like gotos and I'm not a big fan.

pkg/storage/allocator_test.go, line 1042 at r1 (raw file):

I find it easier to pinpoint a test case when there are a large number of them. But with such a small number, this is fine.

Comments from Reviewable

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

pkg/storage/allocator.go, line 449 at r1 (raw file):

This code needs to be adapted to the rule-allocator very soon. Any significant usage of diversity or constraints will foul up lease rebalancing.

Comments from Reviewable