cmd: add the ability to specify localities in allocsim

[BramGruneir]

This change is 

[petermattis]

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

---

pkg/cmd/allocsim/main.go, line 196 at r1 (raw file):

			loc = fmt.Sprintf(":%s", localities[i-1])
		}
		node := fmt.Sprintf("%d%s", i, loc)

Seems more natural to append to node if a locality exists:

node := fmt.Sprintf("%d")
if i <= len(localities) {
  node += fmt.Sprintf(":%s", localities[i-1])
}

---

pkg/cmd/allocsim/main.go, line 296 at r1 (raw file):

	a := newAllocSim(c)

	var localities [][]string

Somewhat confusing to have localities and a.localities that are so similar in name but have different contents. I think s/localities/perNodeArgs/g would make this clearer.

This might be better as map[int][]string as the lookup of the per-node args then becomes: args := perNodeArgs[i] which works even if the map is nil.

---

pkg/cmd/allocsim/main.go, line 299 at r1 (raw file):

	if *numLocalities > 0 {
		a.localities = make([]roachpb.Locality, len(c.Nodes), len(c.Nodes))
		// Add some localities to the cluster. The

Broken sentence: The ?.

---

pkg/cmd/allocsim/main.go, line 328 at r1 (raw file):

	}()

	c.Start("allocsim", *workers, []string{}, flag.Args(), localities)

The third argument can be nil instead of an empty []string.

---

pkg/cmd/internal/localcluster/localcluster.go, line 117 at r1 (raw file):

	for i := range c.Nodes {
		nodeArgs := allNodeArgs

It is subtle that this works (and you should definitely understand why it is subtle). You should be making a copy of allNodeArgs:

nodeArgs := append([]string(nil), allNodeArgs...)

---

pkg/cmd/internal/localcluster/localcluster.go, line 120 at r1 (raw file):

		if perNodeArgs != nil {
			nodeArgs = append(nodeArgs, perNodeArgs[i]...)
		}

See other comment about changing the type of perNodeArgs to map[int][]string. The above would then become:

nodeArgs := append([]string(nil), allNodeArgs...)
nodeArgs = append(nodeArgs, perNodeArgs[i]...)

---

pkg/cmd/internal/localcluster/localcluster.go, line 393 at r1 (raw file):

	}

	log.Infof(context.Background(), "Starting Node: %s\n", n.args)

Did you mean to leave this in? The command line is already printed in Node.Start.

Also s/\n//g.

---

Comments from Reviewable

[BramGruneir]

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

---

pkg/cmd/allocsim/main.go, line 196 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Seems more natural to append to node if a locality exists:

node := fmt.Sprintf("%d")
if i <= len(localities) {
  node += fmt.Sprintf(":%s", localities[i-1])
}

Done.

---

pkg/cmd/allocsim/main.go, line 296 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Somewhat confusing to have localities and a.localities that are so similar in name but have different contents. I think s/localities/perNodeArgs/g would make this clearer.

This might be better as map[int][]string as the lookup of the per-node args then becomes: args := perNodeArgs[i] which works even if the map is nil.

Done.

---

pkg/cmd/allocsim/main.go, line 299 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Broken sentence: The ?.

Just removed the comment entirely.

---

pkg/cmd/allocsim/main.go, line 328 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The third argument can be nil instead of an empty []string.

Done.

---

pkg/cmd/internal/localcluster/localcluster.go, line 117 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

It is subtle that this works (and you should definitely understand why it is subtle). You should be making a copy of allNodeArgs:

nodeArgs := append([]string(nil), allNodeArgs...)

Indeed. It was bugging me.

---

pkg/cmd/internal/localcluster/localcluster.go, line 120 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

See other comment about changing the type of perNodeArgs to map[int][]string. The above would then become:

nodeArgs := append([]string(nil), allNodeArgs...)
nodeArgs = append(nodeArgs, perNodeArgs[i]...)

Done.

---

pkg/cmd/internal/localcluster/localcluster.go, line 393 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Did you mean to leave this in? The command line is already printed in Node.Start.

Also s/\n//g.

nope, left in from testing. Removed.

---

Comments from Reviewable

[petermattis]

---

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable