storage: remove pointers from allocator returns

[BramGruneir]

Specifically, remove them from RebalanceTarget and AllocateTarget.

Part of #10275.

---

This change is 

[petermattis]

I'm missing the motivation for this change. Returning a pointer nicely captures the optionality of the return value. And returning the structure is actually a pessimization as it consumes more stack space (which isn't a big deal for this code, but still).

---

Review status: 0 of 4 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[tamird]

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

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

	for attrs := constraints.Constraints; ; attrs = attrs[:len(attrs)-1] {
		filteredSL := sl.filter(config.Constraints{Constraints: attrs})
		if target := a.selectGood(filteredSL, existingNodes); target != nil {

why not propagate the move to values to selectGood?

---

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

	leaseStoreID roachpb.StoreID,
	rangeID roachpb.RangeID,
) (bool, roachpb.StoreDescriptor, error) {

why the unusual order of returns?

---

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

		existingNodes[repl.NodeID] = struct{}{}
	}
	improve := a.improve(sl, existingNodes)

why not propagate the move to values to improve?

---

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

			false,
		)
		if result.StoreID != 0 {

result != (roachpb.ReplicaDescriptor{})

---

Comments from Reviewable

[BramGruneir]

Unless I'm mistaken, using a pointer to show that a value was retrieved or not is considered a go anti-pattern and the preferred pattern is to return a boolean as well.

---

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

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

Previously, tamird (Tamir Duberstein) wrote…

> why not propagate the move to values to `selectGood`?

Seemed like it was unnecessary considering that we are going to be moving to the rule solver. But since this might be a while. Done.

---

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

Previously, tamird (Tamir Duberstein) wrote…

> why the unusual order of returns?

Done.

---

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

Previously, tamird (Tamir Duberstein) wrote…

> why not propagate the move to values to `improve`?

Done.

---

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

Previously, tamird (Tamir Duberstein) wrote…

> `result != (roachpb.ReplicaDescriptor{})`

Had to use deepEqual. :(

---

Comments from Reviewable

[petermattis]

Unless I'm mistaken, using a pointer to show that a value was retrieved or not is considered a go anti-pattern and the preferred pattern is to return a boolean as well.

I'm not aware of that. The Go stdlib certainly uses sentinels instead of a separate return value in various places. For example, strings.Index returns -1 if the string can't be found. I don't see a significant difference in using a nil pointer return value to indicate not found. I'm thumbs down on this change, particularly now when we have so many other moving parts in this area of the code.

[petermattis]

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

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

Previously, BramGruneir (Bram Gruneir) wrote…

> Had to use deepEqual. :(

Yet another reason against this change.

---

Comments from Reviewable

[tamird]

I think those examples from the standard library aren't exactly canon - there's a lot of junk in the stdlib that was written early and can't be changed. That said, I'm also thumbs down on this as-written for reasons elaborated on in another comment.

---

Reviewed 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

pkg/storage/allocator.go, line 484 at r2 (raw file):

		existingNodes[repl.NodeID] = struct{}{}
	}
	if improve, ok := a.improve(sl, existingNodes); ok {

isn't this equivalent to

improve, ok := a.improve(sl, existingNodes)
return improve, ok, nil

---

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

Previously, petermattis (Peter Mattis) wrote…

> Yet another reason against this change.

Yeah, this stinks - changing things to return values only makes sense when you're able to change the entire chain - that is, if you're dereferencing anything _anywhere_ to do it, you've probably already pessimized it. The same is true of the use of a boolean - either use it everywhere, or you end up with fragile or slow patterns like `reflect.DeepEqual()` or `.StoreID == 0`.

---

pkg/storage/balancer.go, line 42 at r2 (raw file):

func formatCandidates(
	selected roachpb.StoreDescriptor, candidates []roachpb.StoreDescriptor,

another example of an interface that would need to change if this were to move to values - you'd probably want to pass selected's index in candidates rather than the value itself.

---

pkg/storage/balancer.go, line 103 at r2 (raw file):

		// than the average numbers of ranges.
		candidates := make([]roachpb.StoreDescriptor, 0, len(sl.stores))
		for i := range sl.stores {

for _, candidate := range ...?

---

pkg/storage/balancer.go, line 115 at r2 (raw file):

			// candidates.
			bad = candidates[rcb.rand.Intn(len(candidates))]
			ok = true

this is odd. bad and ok are way more broadly scoped than they need to be.

---

Comments from Reviewable

[petermattis]

I think those examples from the standard library aren't exactly canon - there's a lot of junk in the stdlib that was written early and can't be changed.

No, those examples aren't canon and neither is the stdlib, but returning pointers is very common. I don't think we should be changing code like this unless there is a very compelling reason. For example, a nil return value doesn't make sense or returning a pointer incurs an allocation which has been shown to be costly via profiling.

---

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[tamird]

Yes, that's a sensible argument. I don't particularly feel strongly about this change one way or the other, but I wanted to enumerate the rationale one might consider.

…

On Nov 23, 2016 20:30, "Peter Mattis" ***@***.***> wrote: I think those examples from the standard library aren't exactly canon - there's a lot of junk in the stdlib that was written early and can't be changed. No, those examples aren't canon and neither is the stdlib, but returning pointers is very common. I don't think we should be changing code like this unless there is a very compelling reason. For example, a nil return value doesn't make sense or returning a pointer incurs an allocation which has been shown to be costly via profiling. ------------------------------ Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. ------------------------------ *Comments from Reviewable <https://reviewable.io:443/reviews/cockroachdb/cockroach/11206#-:-KXIwdti3X89sSV0b-OW:b-yww06z>* — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#11206 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPE1PelDxv3o5BTHxYYhNrCWTJArZks5rBOjHgaJpZM4K65dV> .

[BramGruneir]

ok, I'm going to close this. Maybe revisit it once the rest of the changes to allocator are complete.