ui, server, storage: add command queue debug report

[vilterp]

Fixes #9797

[cockroach-teamcity]

This change is 

[vilterp]

Working on:

• Getting the command queue data exported via the new endpoint
• Loading it into the JS data model
• Displaying it in the UI

[nvanbenschoten]

cockroach/pkg/util/interval/interval.go

Lines 175 to 180 in bddd7c1

	// Do performs fn on all intervals stored in the tree. The traversal is done
	// in the nondecreasing order of interval start. A boolean is returned
	// indicating whether the traversal was interrupted by an Operation returning
	// true. If fn alters stored intervals' sort relationships, future tree
	// operation behaviors are undefined.
	Do(fn Operation) bool

is probably what you're looking for.

[vilterp]

Perfect, thx

[spencerkimball]

I find it super helpful in these PRs when there's an included screen shot for the UI changes.

[vilterp]

Yeah, I'm a fan of those as well. Will add one when the UI is a little more built out (currently it's just dumping raw JSON response from endpoint).

[vilterp]

@nvanbenschoten The basic endpoint is working. Let's go over this at some point to make sure it's on track, since I'm still not very familiar with these data structures and am probably making some silly mistakes! Haven't gotten to the flattening out parent & child stuff yet.

Let's also talk a little more about what would be useful for the UI. Maybe something pretty similar to the query plan visualizer would be good; I don't know.

[BramGruneir]

I think something like this might be pretty easy to do:
http://bl.ocks.org/d3noob/8326869

Since we already are using d3.

[vilterp]

Used dagre-layout; this is what it looks like now: (hover over a node for details)

[petermattis]

@vilterp That is looking very cool. Keep in mind how this will look/work when there are large numbers of commands in the queue. I suspect the current layout might get overly wide.

[vilterp]

Made it more compact. (The operation ids weren't really serving a purpose in the viz)

[tbg]

Just a random comment on the timestamp field: I think most usable would be <walltime>.<logical> (human-readable wall time). Can also have the human readable in its own row in the table.

[tbg]

Reviewed 1 of 12 files at r1, 4 of 12 files at r4, 1 of 3 files at r5, 1 of 3 files at r6, 2 of 9 files at r7, 1 of 6 files at r8, 2 of 3 files at r11, 1 of 8 files at r15, 12 of 12 files at r18.
Review status: all files reviewed at latest revision, 12 unresolved discussions, some commit checks failed.

---

pkg/roachpb/data.go, line 1190 at r18 (raw file):

// TODO(vilterp) is this an appropriate place for this?
// only used by the storage package for the command queue
func AsSpan(r interval.Range) *Span {

I'd just put this local to your code that needs it.

---

pkg/server/status.go, line 1109 at r18 (raw file):

	var replica *storage.Replica

	// TODO: move this into a `GetReplicaForRange` method on Store?

Always put a (in this case your?) name into TODOs.
I agree that a helper for this should be added to *Stores (mainly because I also use it in a PR I haven't opened yet).

---

pkg/storage/command_queue.proto, line 38 at r18 (raw file):

}

message CommandQueuesForReplica {

You could name this CommandQueueStatus.

---

pkg/storage/replica.go, line 828 at r18 (raw file):

func (r *Replica) setReplicaIDRaftMuLockedMuLocked(replicaID roachpb.ReplicaID) error {
	if r.raftMu.sideloaded == nil || r.mu.replicaID != replicaID {

You seem to have picked up unwanted diff during your rebase.

---

pkg/storage/replica.go, line 851 at r18 (raw file):

	// }

	// Initialize or update the sideloaded storage. If the sideloaded storage

You seem to have picked up unwanted diff during your rebase.

---

pkg/storage/replica.go, line 3252 at r18 (raw file):

		}

		if lastIndex, err = r.raftMu.stateLoader.loadLastIndex(ctx, r.store.Engine()); err != nil {

You seem to have picked up unwanted diff during your rebase.

---

pkg/storage/replica.go, line 3335 at r18 (raw file):

	// Update protected state (last index, raft log size and raft leader ID) and
	// set raft log entry cache. We clear any older, uncommitted log entries and
	// cache the latest ones.

You seem to have picked up unwanted diff during your rebase.

---

pkg/storage/replica.go, line 3429 at r18 (raw file):

				if len(encodedCommand) == 0 {
					commandID = ""
				} else if err := command.Unmarshal(encodedCommand); err != nil {

You seem to have picked up unwanted diff during your rebase.

---

pkg/storage/replica.go, line 3456 at r18 (raw file):

		case raftpb.EntryConfChange:
			var cc raftpb.ConfChange
			if err := cc.Unmarshal(e.Data); err != nil {

You seem to have picked up unwanted diff during your rebase.

---

pkg/storage/replica.go, line 3460 at r18 (raw file):

			}
			var ccCtx ConfChangeContext
			if err := ccCtx.Unmarshal(cc.Context); err != nil {

You seem to have picked up unwanted diff during your rebase.

---

pkg/storage/replica.go, line 3464 at r18 (raw file):

			}
			var command storagebase.RaftCommand
			if err := command.Unmarshal(ccCtx.Payload); err != nil {

You seem to have picked up unwanted diff during your rebase.

---

Comments from Reviewable

[bdarnell]

Reviewed 1 of 12 files at r1, 4 of 12 files at r4, 1 of 3 files at r5, 1 of 3 files at r6, 2 of 9 files at r7, 1 of 6 files at r8, 2 of 3 files at r11, 1 of 8 files at r15, 12 of 12 files at r18.
Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

---

pkg/roachpb/data.go, line 1190 at r18 (raw file):

// TODO(vilterp) is this an appropriate place for this?
// only used by the storage package for the command queue
func AsSpan(r interval.Range) *Span {

I'm fine putting it here next to Span.AsRange, but since it's a package-level function the name should be something like roachpb.RangeToSpan(r) instead of AsSpan.

---

pkg/storage/command_queue.proto, line 32 at r18 (raw file):

// would be called CommandQueue but that's already taken by the struct in command_queue.go
// TODO(vilterp): should we use the same proto to define that struct that we use for

Can this comment be removed now that you're referencing this proto from the server one?

---

pkg/storage/replica.go, line 678 at r18 (raw file):

		return err
	}
	r.mu.lastTerm = invalidLastTerm

This constant was just introduced recently (by @nvanbenschoten ). I don't feel strongly about whether we use a named constant or a bare zero (I have a slight preference for the bare zero), but we shouldn't flip back and forth on this.

---

pkg/ui/src/redux/cachedDataReducer.ts, line 110 at r18 (raw file):

        state.inFlight = false;
        state.lastError = error.data;
        console.error(error);

Did you mean to leave this in?

---

pkg/ui/src/views/reports/containers/commandQueue/index.tsx, line 20 at r18 (raw file):

type CommandQueueProps = CommandQueueOwnProps & RouterState;

// function ErrorPage(props: {

Remove this or uncomment and use it.

---

Comments from Reviewable

[vilterp]

Review status: 15 of 25 files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

---

pkg/storage/command_queue.proto, line 32 at r18 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Can this comment be removed now that you're referencing this proto from the server one?

was wondering if I should use a proto here instead of the CommandQueue struct defined command_queue.go. Seems not worth it

---

pkg/storage/command_queue.proto, line 38 at r18 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

You could name this CommandQueueStatus.

not sure if that's different enough than CommandQueueSnapshot; leaving for now

---

pkg/roachpb/data.go, line 1190 at r18 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I'm fine putting it here next to Span.AsRange, but since it's a package-level function the name should be something like roachpb.RangeToSpan(r) instead of AsSpan.

ended up not using it; removing in next commit

---

Comments from Reviewable

[bdarnell]

Review status: 15 of 25 files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

---

pkg/storage/command_queue.proto, line 32 at r18 (raw file):

Previously, vilterp (Pete Vilter) wrote…

was wondering if I should use a proto here instead of the CommandQueue struct defined command_queue.go. Seems not worth it

I don't understand. There's a lot more in storage.CommandQueue than a list of commands. I thought you were talking about this CommandQueueCommand vs storage.cmd.

---

Comments from Reviewable

[couchand]

Mainly reviewed the front-end changes. Good work on the whole. I left lots of little comments.

---

Reviewed 1 of 12 files at r1, 2 of 12 files at r4, 1 of 3 files at r5, 1 of 3 files at r6, 3 of 12 files at r18, 3 of 4 files at r21, 1 of 4 files at r23, 3 of 5 files at r24.
Review status: 18 of 25 files reviewed at latest revision, 33 unresolved discussions, some commit checks failed.

---

pkg/ui/webpack.config.js, line 42 at r26 (raw file):

    // for a "node_modules" child directory located in either the current
    // directory *or in any parent directory*.
    modules: [path.resolve(__dirname), "node_modules"],

If this change is necessary, it would seem to be wise to update the comment. However, reading the comment seems to indicate that it would be a good idea to keep the original. What motivated the change?

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 7 at r26 (raw file):

import * as dagre from "dagre";

import * as protos from "src/js/protos";

Please sort your imports, and group them as external first and internal second.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 54 at r26 (raw file):

  }

  detailsTable(command: protos.cockroach.storage.CommandQueueCommand$Properties) {

might be nice to prefix this method with "render"

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 59 at r26 (raw file):

        <tbody>
          <tr>
            <td>ID</td>

Maybe these should be th?

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 60 at r26 (raw file):

          <tr>
            <td>ID</td>
            <td>{`${command.id.toString()}`}</td>

The backtick string isn't needed here.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 64 at r26 (raw file):

          <tr>
            <td>Read Only</td>
            <td>{`${command.readonly}`}</td>

ditto.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 87 at r26 (raw file):

    const g = this.computeLayout();

    const nodeIds = g.nodes() as Array<string>;

Looks like this is only used on the line below, maybe inline it?

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 88 at r26 (raw file):

    const nodeIds = g.nodes() as Array<string>;
    const nodes = nodeIds.map((nodeId) => (

These functions would probably read nicer one-lined.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 109 at r26 (raw file):

          {nodes.map((node) => {
            if (!node.command) {
              console.log("no command", node);

Is this for debugging?

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 117 at r26 (raw file):

                cy={node.y}
                r={COMMAND_RADIUS}
                onClick={() => { this.setState({ hoveredNode: node.command.id }); }}

Seems odd to call it "hoveredNode", then.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 131 at r26 (raw file):

                key={idx}
                points={
                  (edge.points as Array<{x: number, y: number}>)

This cast might read nicer if you did it up above the return.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 138 at r26 (raw file):

          })}
        </svg>
        {this.state.hoveredNode !== null

Ugh, multi-line ternary! Maybe there's a way to avoid that?

---

pkg/ui/src/views/reports/containers/commandQueue/index.tsx, line 51 at r26 (raw file):

        </h1>
        <div className="command-queue__timestamp">
          {this.props.commandQueue

Oh no, another multi-line ternary! In this case, there's definitely an improvement to be made by showing just one Loading... while loading with an early return rather than conditionally rendering that all over.

---

pkg/ui/src/views/reports/containers/commandQueue/index.tsx, line 54 at r26 (raw file):

            ? <span>
                Snapshot taken at
                {" "}{LongToMoment(this.props.commandQueue.queues.timestamp).toISOString()}

Maybe use Print.Timestamp to keep the format consistent?

---

pkg/ui/src/views/reports/containers/range/rangeTable.tsx, line 341 at r26 (raw file):

        {sortedStoreIDs.map((storeId) => (
          <td className="range-table__cell" key={storeId}>
            {storeId === leader.source_store_id

Another multi-line ternary!

---

pkg/ui/styl/pages/command_queue.styl, line 15 at r26 (raw file):

.command-queue__key__read
  background-color lightgreen

I'm wondering if we want to be using colors from the palette for these? It would probably also be nice to apply the same classes to the nodes so that the coloring isn't in two different places.

---

Comments from Reviewable

[vilterp]

Reviewed 5 of 12 files at r1, 12 of 12 files at r4, 3 of 3 files at r5, 3 of 3 files at r6, 9 of 9 files at r7, 6 of 6 files at r8, 2 of 2 files at r9, 2 of 2 files at r10, 3 of 3 files at r11, 1 of 1 files at r12, 4 of 5 files at r13, 2 of 3 files at r14, 7 of 8 files at r15, 1 of 1 files at r16, 1 of 1 files at r17, 12 of 12 files at r18, 1 of 1 files at r19, 3 of 3 files at r20, 4 of 4 files at r21, 2 of 2 files at r22, 4 of 4 files at r23, 5 of 5 files at r24, 2 of 2 files at r25, 1 of 1 files at r26, 2 of 2 files at r27.
Review status: 22 of 27 files reviewed at latest revision, 33 unresolved discussions, some commit checks failed.

---

pkg/server/status.go, line 1109 at r18 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Always put a (in this case your?) name into TODOs.
I agree that a helper for this should be added to *Stores (mainly because I also use it in a PR I haven't opened yet).

Done.

---

pkg/storage/command_queue.proto, line 32 at r18 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I don't understand. There's a lot more in storage.CommandQueue than a list of commands. I thought you were talking about this CommandQueueCommand vs storage.cmd.

Discussed in person: we'll stick with having separate Go structs (cmd and CommandQueue) and protos (CommandQueueCommand and CommandQueueSnapshot) since the Go structs have a lot of stuff in them the UI doesn't care about and there's no straightforward way to embed the proto structs.

---

pkg/storage/replica.go, line 678 at r18 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

This constant was just introduced recently (by @nvanbenschoten ). I don't feel strongly about whether we use a named constant or a bare zero (I have a slight preference for the bare zero), but we shouldn't flip back and forth on this.

Had no intention to change this; somehow messed up rebase. Reverted my change so it'll be left as is.

---

pkg/storage/replica.go, line 828 at r18 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

You seem to have picked up unwanted diff during your rebase.

Done.

---

pkg/storage/replica.go, line 851 at r18 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

You seem to have picked up unwanted diff during your rebase.

Done.

---

pkg/storage/replica.go, line 3252 at r18 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

You seem to have picked up unwanted diff during your rebase.

Done.

---

pkg/storage/replica.go, line 3335 at r18 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

You seem to have picked up unwanted diff during your rebase.

Done.

---

pkg/storage/replica.go, line 3429 at r18 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

You seem to have picked up unwanted diff during your rebase.

Done.

---

pkg/storage/replica.go, line 3456 at r18 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

You seem to have picked up unwanted diff during your rebase.

Done.

---

pkg/storage/replica.go, line 3460 at r18 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

You seem to have picked up unwanted diff during your rebase.

Done.

---

pkg/storage/replica.go, line 3464 at r18 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

You seem to have picked up unwanted diff during your rebase.

Done.

---

pkg/ui/webpack.config.js, line 42 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

If this change is necessary, it would seem to be wise to update the comment. However, reading the comment seems to indicate that it would be a good idea to keep the original. What motivated the change?

This change was so that graphlib, a dependency of dagre (DAG layout lib added in this change) could use Lodash 3 (installed in node_modules/graphlib/node_modules/lodash) instead of Lodash 4 (installed at node_modules/lodash). Without this change, webpack would resolve dagre's import of lodash to Lodash 3 (the one at the top level) and then graphlib would explode because it would call Lodash 4 thinking it was Lodash 3 (there were some breaking API changes in _.filter).

@mrtracy and I came up with this fix but forgot to update the comment. I think node's behavior is what we want — i.e. don't think it's a problem that it looks at parent directories. Will just update it to reflect that we're using relative path for one and absolute for other.

---

pkg/ui/src/redux/cachedDataReducer.ts, line 110 at r18 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Did you mean to leave this in?

removed

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 7 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

Please sort your imports, and group them as external first and internal second.

Done. Wish we had a tool which did this automatically!

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 54 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

might be nice to prefix this method with "render"

Done.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 59 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

Maybe these should be th?

Done.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 60 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

The backtick string isn't needed here.

Done. Derp!

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 64 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

ditto.

Done.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 87 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

Looks like this is only used on the line below, maybe inline it?

Done.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 88 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

These functions would probably read nicer one-lined.

Done.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 109 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

Is this for debugging?

Yeah. Seemed to be a case where there was no command and stuff below exploded will take it out; hopefully I fixed that elsewhere.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 117 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

Seems odd to call it "hoveredNode", then.

Indeed! Used to be on hover. Will change it.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 131 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

This cast might read nicer if you did it up above the return.

Done.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 138 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

Ugh, multi-line ternary! Maybe there's a way to avoid that?

Done.

---

pkg/ui/src/views/reports/containers/commandQueue/index.tsx, line 20 at r18 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Remove this or uncomment and use it.

Done.

---

pkg/ui/src/views/reports/containers/commandQueue/index.tsx, line 51 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

Oh no, another multi-line ternary! In this case, there's definitely an improvement to be made by showing just one Loading... while loading with an early return rather than conditionally rendering that all over.

Agreed; done

---

pkg/ui/src/views/reports/containers/commandQueue/index.tsx, line 54 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

Maybe use Print.Timestamp to keep the format consistent?

Done.

---

pkg/ui/src/views/reports/containers/range/rangeTable.tsx, line 341 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

Another multi-line ternary!

Done.

---

pkg/ui/styl/pages/command_queue.styl, line 15 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

I'm wondering if we want to be using colors from the palette for these? It would probably also be nice to apply the same classes to the nodes so that the coloring isn't in two different places.

Done.

---

pkg/roachpb/data.go, line 1190 at r18 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

I'd just put this local to your code that needs it.

removed it

---

Comments from Reviewable

[nvanbenschoten]

Reviewed 1 of 12 files at r1, 3 of 12 files at r4, 1 of 6 files at r8, 2 of 4 files at r23, 1 of 5 files at r24, 1 of 2 files at r25, 1 of 1 files at r26, 2 of 2 files at r27, 1 of 3 files at r29.
Review status: 22 of 26 files reviewed at latest revision, 51 unresolved discussions, some commit checks failed.

---

pkg/server/status.go, line 1104 at r29 (raw file):

}

func (s *statusServer) CommandQueue(

Add a comment on this handler method.

---

pkg/server/status.go, line 1114 at r29 (raw file):

	queueState := replica.GetCommandQueueState()

	response := &serverpb.CommandQueueResponse{

nit: Just inline this variable

---

pkg/server/serverpb/status.proto, line 25 at r29 (raw file):

import "cockroach/pkg/storage/engine/enginepb/mvcc.proto";
import "cockroach/pkg/storage/log.proto";
import "cockroach/pkg/storage/command_queue.proto";

Keep this sorted.

---

pkg/storage/command_queue.go, line 568 at r29 (raw file):

}

func (cq *CommandQueue) flushReadsBuffer() {

Add a comment about what this does.

Also, let's move this up to right after CommandQueue.expand since they're similar methods.

---

pkg/storage/command_queue.go, line 811 at r29 (raw file):

// GetCommandQueueState returns a snapshot of this command queue's state.
func (cq *CommandQueue) GetCommandQueueState() *CommandQueueSnapshot {
	// make sure all operations are in the interval trees

You should read over https://github.com/golang/go/wiki/CodeReviewComments.

Specifically here, we always use full sentences and proper punctuation in comments.

---

pkg/storage/command_queue.go, line 813 at r29 (raw file):

	// make sure all operations are in the interval trees
	cq.flushReadsBuffer()
	result := []*CommandQueueCommand{}

We know the sizes of cq.reads and cq.writes, right? So we can allocate a proper capacity for this slice and save unnecessary reallocations during calls to append.

Try results := make([]*CommandQueueCommand, 0, cq.reads.Len() + cq.writes.Len()).

If you ever know roughly how many elements will be appended to a slice, allocate the capacity ahead of time. But if you don't, don't even allocate the slice at all ahead of time, like var result []*CommandQueueCommand.

---

pkg/storage/command_queue.go, line 824 at r29 (raw file):

	commandsSoFar []*CommandQueueCommand, tree interval.Tree,
) []*CommandQueueCommand {
	result := commandsSoFar

Here and below, is result := commandsSoFar needed? Seems like a useless variable.

---

pkg/storage/command_queue.go, line 833 at r29 (raw file):

}

func copyCommandsAndChildren(

Add a comment since this isn't trivial. Also, is copyCommandsAndChildren the right name? If a command has children then we don't copy it. Perhaps just appendCommand.

---

pkg/storage/command_queue.go, line 839 at r29 (raw file):

	if len(command.children) > 0 {
		for _, childCmd := range command.children {
			result = copyCommandsAndChildren(result, &childCmd)

This is going to allocate because we're taking the address of a local iterator variable. But

for i := range command.children {
	result = copyCommandsAndChildren(result, &command.children[i])
}

won't. You'll get a feel for this as you go, but the general rule of thumb is that "taking the address of something on the stack forces it onto the heap, but taking the address of something already on the heap is fine".

---

pkg/storage/command_queue.go, line 854 at r29 (raw file):

		commandProto.Prereqs = append(commandProto.Prereqs, prereqCmd.id)
	}
	result = append(commandsSoFar, commandProto)

return append(commandsSoFar, commandProto)

---

pkg/storage/command_queue.proto, line 38 at r18 (raw file):

Previously, vilterp (Pete Vilter) wrote…

not sure if that's different enough than CommandQueueSnapshot; leaving for now

What about CommandQueuesStatus. I don't like CommandQueuesForReplica either.

---

pkg/storage/command_queue.proto, line 1 at r29 (raw file):

// Copyright 2015 The Cockroach Authors.

2017

---

pkg/storage/command_queue.proto, line 22 at r29 (raw file):

import "cockroach/pkg/util/hlc/timestamp.proto";

message CommandQueueCommand {

I don't love that a .proto file meant for the UI is leaking into the storage package. We have enough in here. @BramGruneir what have we done for protos like this in the past?

---

pkg/storage/command_queue.proto, line 27 at r29 (raw file):

  string end_key = 3;
  bool readonly = 4;
  util.hlc.Timestamp timestamp = 5;

Take a look at gogoproto. Specifically how it can be used to avoid pointer elements by using [(gogoproto.nullable) = false]. I think we should do that for CommandQueueCommand.timestamp and CommandQueueSnapshot.commands.

---

pkg/storage/command_queue.proto, line 37 at r29 (raw file):

message CommandQueuesForReplica {
  // Timestamp in nanoseconds when this snapshot was taken
  int64 timestamp = 1;

Why not util.hlc.Timestamp?

---

pkg/storage/command_queue_test.go, line 739 at r29 (raw file):

	snapshot1 := cq1.GetCommandQueueState()
	if n1 := len(snapshot1.Commands); n1 != 2 {

We should test more than just the length.

---

pkg/storage/replica.go, line 5626 at r29 (raw file):

	defer r.cmdQMu.Unlock()
	return &CommandQueuesForReplica{
		Timestamp:   timeutil.Now().UnixNano(),

I think you want the timestamp from the Replica's actual HLC clock, right? I'm not actually sure how the UI deals with time. @BramGruneir?

---

pkg/storage/stores.go, line 142 at r29 (raw file):

	var replica *Replica

	err := ls.VisitStores(func(store *Store) error {

Check out the if err := ...; err != nil { pattern.

---

pkg/storage/stores.go, line 143 at r29 (raw file):

	err := ls.VisitStores(func(store *Store) error {
		replicaFromStore, err := store.GetReplica(rangeID)

Why are we ignoring this error?

---

Comments from Reviewable

[mrtracy]

Review status: 22 of 26 files reviewed at latest revision, 51 unresolved discussions, some commit checks failed.

---

pkg/ui/webpack.config.js, line 42 at r26 (raw file):

Previously, vilterp (Pete Vilter) wrote…

This change was so that graphlib, a dependency of dagre (DAG layout lib added in this change) could use Lodash 3 (installed in node_modules/graphlib/node_modules/lodash) instead of Lodash 4 (installed at node_modules/lodash). Without this change, webpack would resolve dagre's import of lodash to Lodash 3 (the one at the top level) and then graphlib would explode because it would call Lodash 4 thinking it was Lodash 3 (there were some breaking API changes in _.filter).

@mrtracy and I came up with this fix but forgot to update the comment. I think node's behavior is what we want — i.e. don't think it's a problem that it looks at parent directories. Will just update it to reflect that we're using relative path for one and absolute for other.

Okay, this has a complicated story behind it. But tldr; What you see here, the version in this commit, is how it always should have been, the old version has problems.

Webpack's default value for this is modules: ["node_modules"], which performs the normal node-style resolution for non-relative modules (i.e. checking in "node_modules", then "../node_modules", then "../../node_modules", etc.). This is needed to support node's multi-version dependency management; the addition of of graphlib depends on that to work.

What we wanted to do was modify this resolution to check the project root directory first. This lets us reference our own local files using non-relative paths (e.g. "src/redux/nodes.ts" instead of "../../redux/nodes.ts"), which is great for refactoring, grepping, etc. We do this by adding the absolute path of the root directory (path.resolve(__dirname)) before "node_modules" in the modules setting. With this, if a file has a statement import * from "foo", webpack will look for the "foo" module in the following locations, in order:

1. /absolute/path/to/project/foo
2. ./node_modules/foo
3. ../node_modules/foo
4. ../../node_modules/foo

...etc.

However, why did we make "node_modules" also an absolute path? This breaks dependency management, so there must have been a reason.

Well, the reason is that this mirrors the resolution strategy specified in tsconfig.json, which is used by things like command-line tsc and IDEs. Tsconfig has more limited resolution flexibility than webpack, and can't mix absolute-path resolution with node-style resolution. So, in order to match these strategies exactly, we made webpack's resolution more strict than it needed to be.

However, I now believe that to have been a mistake, for the reason highlighted in this PR (breaking node's dependency management). Instead, we are just going to have to live with the slight difference in resolution strategy between the two files. Although, since our typescript has no need for multi-version dependency management, there should currently be no issue.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 7 at r26 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Done. Wish we had a tool which did this automatically!

I've filed an issue for this. tslint can enforce the sorting, but we're on our own in terms of grouping, and it's not automatic like gofmt.

---

Comments from Reviewable

[vilterp]

Reviewed 1 of 12 files at r18, 5 of 5 files at r28, 2 of 3 files at r29.
Review status: 9 of 26 files reviewed at latest revision, 50 unresolved discussions, some commit checks failed.

---

pkg/server/status.go, line 1104 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Add a comment on this handler method.

Done.

---

pkg/server/status.go, line 1114 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: Just inline this variable

Done.

---

pkg/server/serverpb/status.proto, line 25 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Keep this sorted.

Done. Ended up removing the new import.

---

pkg/storage/command_queue.go, line 568 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Add a comment about what this does.

Also, let's move this up to right after CommandQueue.expand since they're similar methods.

Done.

---

pkg/storage/command_queue.go, line 811 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

You should read over https://github.com/golang/go/wiki/CodeReviewComments.

Specifically here, we always use full sentences and proper punctuation in comments.

Done.

---

pkg/storage/command_queue.go, line 813 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We know the sizes of cq.reads and cq.writes, right? So we can allocate a proper capacity for this slice and save unnecessary reallocations during calls to append.

Try results := make([]*CommandQueueCommand, 0, cq.reads.Len() + cq.writes.Len()).

If you ever know roughly how many elements will be appended to a slice, allocate the capacity ahead of time. But if you don't, don't even allocate the slice at all ahead of time, like var result []*CommandQueueCommand.

Done.

---

pkg/storage/command_queue.go, line 824 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Here and below, is result := commandsSoFar needed? Seems like a useless variable.

Done.

---

pkg/storage/command_queue.go, line 833 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Add a comment since this isn't trivial. Also, is copyCommandsAndChildren the right name? If a command has children then we don't copy it. Perhaps just appendCommand.

Done.

---

pkg/storage/command_queue.go, line 839 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This is going to allocate because we're taking the address of a local iterator variable. But

for i := range command.children {
	result = copyCommandsAndChildren(result, &command.children[i])
}

won't. You'll get a feel for this as you go, but the general rule of thumb is that "taking the address of something on the stack forces it onto the heap, but taking the address of something already on the heap is fine".

Done.

---

pkg/storage/command_queue.go, line 854 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

return append(commandsSoFar, commandProto)

Done.

---

pkg/storage/command_queue_test.go, line 739 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We should test more than just the length.

Done.

---

pkg/storage/replica.go, line 5626 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I think you want the timestamp from the Replica's actual HLC clock, right? I'm not actually sure how the UI deals with time. @BramGruneir?

Done.

---

pkg/storage/stores.go, line 142 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Check out the if err := ...; err != nil { pattern.

Done.

---

pkg/storage/stores.go, line 143 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why are we ignoring this error?

Done.

---

pkg/storage/command_queue.proto, line 38 at r18 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

What about CommandQueuesStatus. I don't like CommandQueuesForReplica either.

How about getting rid of it and having just two messages, CommandQueueSnapshot and CommandQueueCommand?

---

pkg/storage/command_queue.proto, line 1 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

2017

Done. (moved these to existing file)

---

pkg/storage/command_queue.proto, line 22 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I don't love that a .proto file meant for the UI is leaking into the storage package. We have enough in here. @BramGruneir what have we done for protos like this in the past?

After in-person discussion, moved it to storagebase/state.proto, under RangeInfo, which is also used by the UI

---

pkg/storage/command_queue.proto, line 27 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Take a look at gogoproto. Specifically how it can be used to avoid pointer elements by using [(gogoproto.nullable) = false]. I think we should do that for CommandQueueCommand.timestamp and CommandQueueSnapshot.commands.

Done.

---

pkg/storage/command_queue.proto, line 37 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why not util.hlc.Timestamp?

Done.

---

Comments from Reviewable

[vilterp]

@nvanbenschoten I'm surprised that this command has no prereqs. Shouldn't it have a prereq on the command with id 1? (It's one of the leaf commands generated when a command with two spans is added, with a prereq)

[nvanbenschoten]

Planning to give all these changes a more thorough review tomorrow. For now, double check what snapshotCommands[3] is giving you. snapshotCommands is indexed by the command id, not the index of the command in the snapshot slice. I don't think there is a command with id 0 or 3 so I think you're seeing default values for some of these. I'd expect ids [1, 2, 4, 5] (gap because of covering span).

Also, these tests should assert as much as possible. Try something like:

if expPrereqs := []int64{1}; !reflect.DeepEqual(expPrereqs, snapshotCommands[5].Prereqs) {
    ...
}

[nvanbenschoten]

Reviewed 7 of 18 files at r31, 2 of 2 files at r32, 1 of 3 files at r33.
Review status: 19 of 26 files reviewed at latest revision, 41 unresolved discussions, some commit checks failed.

---

pkg/server/status.go, line 1104 at r29 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Done.

Double check that :)

---

pkg/storage/command_queue.go, line 817 at r33 (raw file):

	// the interval trees.
	cq.flushReadsBuffer()
	commandMap := make(map[int64]*storagebase.CommandQueueCommand)

Using a *storagebase.CommandQueueCommand value type seems wrong to me. I think you're only doing so because you want to modify in-place down in removeNonexistentPrereqs. Perhaps rename that to filterNonexistentPrereqs and have it construct the result slice. Then you'll have less mutation and can simply return filterNonexistentPrereqs(commandMap).

---

pkg/storage/command_queue.go, line 870 at r33 (raw file):

		filteredPrereqs := make([]int64, 0, len(command.Prereqs))
		for _, prereq := range command.Prereqs {
			_, ok := commandMap[prereq]

nit: if _, ok := commandMap[prereq]; ok {

---

pkg/storage/command_queue_test.go, line 749 at r33 (raw file):

		snapshotCommands[command.Id] = command
	}
	if len(snapshotCommands[0].Prereqs) != 0 {

Yeah, let's use the reflect.DeepEqual approach for all of these. It tests more and is easier to read.

---

pkg/storage/replica.go, line 5652 at r33 (raw file):

	return storagebase.CommandQueuesSnapshot{
		Timestamp:   r.store.Clock().Now(),
		LocalScope:  r.cmdQMu.queues[spanLocal].GetCommandQueueSnapshot(),

Having two different methods called GetCommandQueueSnapshot which return different types is confusing. How about we keep this the same and change the other one to

func (cq *CommandQueue) GetSnapshotCommands() []storagebase.CommandQueueCommand

---

pkg/storage/storagebase/state.proto, line 114 at r33 (raw file):

}

message CommandQueuesSnapshot {

Add a comment to this type.

---

pkg/storage/storagebase/state.proto, line 115 at r33 (raw file):

message CommandQueuesSnapshot {
  // Timestamp in nanoseconds when this snapshot was taken

taken.

---

pkg/storage/command_queue.proto, line 38 at r18 (raw file):

Previously, vilterp (Pete Vilter) wrote…

How about getting rid of it and having just two messages, CommandQueueSnapshot and CommandQueueCommand?

Much better.

---

Comments from Reviewable

[couchand]

Reviewed 1 of 12 files at r18, 1 of 5 files at r28, 2 of 7 files at r30, 1 of 18 files at r31, 1 of 3 files at r33.
Review status: 23 of 26 files reviewed at latest revision, 32 unresolved discussions, some commit checks failed.

---

pkg/ui/webpack.config.js, line 42 at r26 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Okay, this has a complicated story behind it. But tldr; What you see here, the version in this commit, is how it always should have been, the old version has problems.

Webpack's default value for this is modules: ["node_modules"], which performs the normal node-style resolution for non-relative modules (i.e. checking in "node_modules", then "../node_modules", then "../../node_modules", etc.). This is needed to support node's multi-version dependency management; the addition of of graphlib depends on that to work.

What we wanted to do was modify this resolution to check the project root directory first. This lets us reference our own local files using non-relative paths (e.g. "src/redux/nodes.ts" instead of "../../redux/nodes.ts"), which is great for refactoring, grepping, etc. We do this by adding the absolute path of the root directory (path.resolve(__dirname)) before "node_modules" in the modules setting. With this, if a file has a statement import * from "foo", webpack will look for the "foo" module in the following locations, in order:

1. /absolute/path/to/project/foo
2. ./node_modules/foo
3. ../node_modules/foo
4. ../../node_modules/foo

...etc.

However, why did we make "node_modules" also an absolute path? This breaks dependency management, so there must have been a reason.

Well, the reason is that this mirrors the resolution strategy specified in tsconfig.json, which is used by things like command-line tsc and IDEs. Tsconfig has more limited resolution flexibility than webpack, and can't mix absolute-path resolution with node-style resolution. So, in order to match these strategies exactly, we made webpack's resolution more strict than it needed to be.

However, I now believe that to have been a mistake, for the reason highlighted in this PR (breaking node's dependency management). Instead, we are just going to have to live with the slight difference in resolution strategy between the two files. Although, since our typescript has no need for multi-version dependency management, there should currently be no issue.

Ok, looking at this again that makes historical sense. There are still two things I don't like:

• path.resolve isn't needed. It should always be the case that __dirname === path.resolve(__dirname).
• We really shouldn't be including two copies of lodash in our bundle. If this dagre library has a hard dependency on an old version of lodash we should try not to include it. Can we use the D3 layout functions instead? They're pretty comprehensive.

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 7 at r26 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

I've filed an issue for this. tslint can enforce the sorting, but we're on our own in terms of grouping, and it's not automatic like gofmt.

eslint has an eslint --fix command that works for most mechanical rules, does tslint not have something comparable?

---

pkg/ui/src/views/reports/containers/commandQueue/index.tsx, line 6 at r33 (raw file):

import React from "react";
import { connect } from "react-redux";
import { RouterState } from "react-router";

Please combine the two imports from react-router.

---

pkg/ui/src/views/reports/containers/commandQueue/index.tsx, line 80 at r33 (raw file):

            r{rangeID.toString()}
          </Link>
          {" "}>{" "}

This would probably read nicer as just {" > "}...

---

Comments from Reviewable

[mrtracy]

Review status: 23 of 26 files reviewed at latest revision, 32 unresolved discussions, some commit checks failed.

---

pkg/ui/webpack.config.js, line 42 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

Ok, looking at this again that makes historical sense. There are still two things I don't like:

• path.resolve isn't needed. It should always be the case that __dirname === path.resolve(__dirname).
• We really shouldn't be including two copies of lodash in our bundle. If this dagre library has a hard dependency on an old version of lodash we should try not to include it. Can we use the D3 layout functions instead? They're pretty comprehensive.

Why is it bad to have two versions of lodash in the bundle? Lodash is a pure utility library, having two versions in the bundle should cause no problems as long as everything gets the version it expects.

If you're just concerned about bundle size, we should file an issue to do an explicit pruning sweep. Without seeing numbers, we can't be certain that Dagre would be major contributor to bloat, and I wouldn't want to derail this PR further without more evidence.

---

Comments from Reviewable

[couchand]

pkg/ui/webpack.config.js, line 42 at r26 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Why is it bad to have two versions of lodash in the bundle? Lodash is a pure utility library, having two versions in the bundle should cause no problems as long as everything gets the version it expects.

If you're just concerned about bundle size, we should file an issue to do an explicit pruning sweep. Without seeing numbers, we can't be certain that Dagre would be major contributor to bloat, and I wouldn't want to derail this PR further without more evidence.

Lodash is our third-biggest dependency, and after NVD3 is gone it will be the second-biggest. I've created an issue as you suggested (#18789), but I also think our standard practice should be to try to make use of the libraries we're already including before reaching for a new one. I'm not sure it really counts as "derailing" - it's entirely part of the responsibility of a PR to justify any new dependency.

---

Comments from Reviewable

[vilterp]

Reviewed 1 of 12 files at r18, 2 of 7 files at r30, 10 of 18 files at r31, 2 of 2 files at r32, 3 of 3 files at r33.
Review status: 16 of 26 files reviewed at latest revision, 30 unresolved discussions, some commit checks failed.

---

pkg/server/status.go, line 1104 at r29 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Double check that :)

Done. Whoops! haha.

---

pkg/storage/command_queue.go, line 817 at r33 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Using a *storagebase.CommandQueueCommand value type seems wrong to me. I think you're only doing so because you want to modify in-place down in removeNonexistentPrereqs. Perhaps rename that to filterNonexistentPrereqs and have it construct the result slice. Then you'll have less mutation and can simply return filterNonexistentPrereqs(commandMap).

Done.

---

pkg/storage/command_queue.go, line 870 at r33 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: if _, ok := commandMap[prereq]; ok {

Done.

---

pkg/storage/command_queue_test.go, line 790 at r32 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Planning to give all these changes a more thorough review tomorrow. For now, double check what snapshotCommands[3] is giving you. snapshotCommands is indexed by the command id, not the index of the command in the snapshot slice. I don't think there is a command with id 0 or 3 so I think you're seeing default values for some of these. I'd expect ids [1, 2, 4, 5] (gap because of covering span).

Also, these tests should assert as much as possible. Try something like:

if expPrereqs := []int64{1}; !reflect.DeepEqual(expPrereqs, snapshotCommands[5].Prereqs) {
    ...
}

Done. Yeah you were totally right. Didn't occur to me that the nil value of a struct is the struct where all of its members are their nil values.

Also didn't know reflect.DeepEquals was a thing; huzzah!

---

pkg/storage/command_queue_test.go, line 749 at r33 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Yeah, let's use the reflect.DeepEqual approach for all of these. It tests more and is easier to read.

Done. Made a helper assertExpectedPrereqs

---

pkg/storage/replica.go, line 5652 at r33 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Having two different methods called GetCommandQueueSnapshot which return different types is confusing. How about we keep this the same and change the other one to

func (cq *CommandQueue) GetSnapshotCommands() []storagebase.CommandQueueCommand

Changed the one in command_queue.go to GetSnapshot, since we already know it's in the context of the command queue.

---

pkg/storage/storagebase/state.proto, line 114 at r33 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Add a comment to this type.

Done.

---

pkg/storage/storagebase/state.proto, line 115 at r33 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

taken.

Done.

---

pkg/ui/webpack.config.js, line 42 at r26 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Why is it bad to have two versions of lodash in the bundle? Lodash is a pure utility library, having two versions in the bundle should cause no problems as long as everything gets the version it expects.

If you're just concerned about bundle size, we should file an issue to do an explicit pruning sweep. Without seeing numbers, we can't be certain that Dagre would be major contributor to bloat, and I wouldn't want to derail this PR further without more evidence.

The only option D3 has for layout of DAGs is force-directed layout which isn't as good as Dagre's (distracting animation at the beginning.

Re: lodash, there's a fork of Dagre called dagre-layout which upgrades the Lodash dep to 4. Only reason I didn't use it was that it doesn't have Typescript bindings, and TSC complains about that. However, dagre's typings are wrong anyway (I have a PR out to fix them); maybe submitting a new typing is just as fast? (It'll be the same as what's in my PR).

---

pkg/ui/src/views/reports/containers/commandQueue/commandQueueViz.tsx, line 109 at r26 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Yeah. Seemed to be a case where there was no command and stuff below exploded will take it out; hopefully I fixed that elsewhere.

(Turned out to be an issue on the Go side; fixed)

---

pkg/ui/src/views/reports/containers/commandQueue/index.tsx, line 6 at r33 (raw file):

Previously, couchand (Andrew Couch) wrote…

Please combine the two imports from react-router.

Done.

---

pkg/ui/src/views/reports/containers/commandQueue/index.tsx, line 80 at r33 (raw file):

Previously, couchand (Andrew Couch) wrote…

This would probably read nicer as just {" > "}...

Done.

---

Comments from Reviewable

[couchand]

Review status: 16 of 26 files reviewed at latest revision, 30 unresolved discussions, some commit checks failed.

---

pkg/ui/webpack.config.js, line 42 at r26 (raw file):

Previously, vilterp (Pete Vilter) wrote…

The only option D3 has for layout of DAGs is force-directed layout which isn't as good as Dagre's (distracting animation at the beginning.

Re: lodash, there's a fork of Dagre called dagre-layout which upgrades the Lodash dep to 4. Only reason I didn't use it was that it doesn't have Typescript bindings, and TSC complains about that. However, dagre's typings are wrong anyway (I have a PR out to fix them); maybe submitting a new typing is just as fast? (It'll be the same as what's in my PR).

You're right that a force layout wouldn't be appropriate. (though there are some techniques for avoiding the initial springing, but that's irrelevant since it still wouldn't look right here).

I think what you're looking for is either cluster (https://github.com/d3/d3-3.x-api-reference/blob/master/Cluster-Layout.md) or tree (https://github.com/d3/d3-3.x-api-reference/blob/master/Tree-Layout.md).

---

Comments from Reviewable

[vilterp]

Review status: 16 of 26 files reviewed at latest revision, 30 unresolved discussions, some commit checks failed.

---

pkg/ui/webpack.config.js, line 42 at r26 (raw file):

Previously, couchand (Andrew Couch) wrote…

You're right that a force layout wouldn't be appropriate. (though there are some techniques for avoiding the initial springing, but that's irrelevant since it still wouldn't look right here).

I think what you're looking for is either cluster (https://github.com/d3/d3-3.x-api-reference/blob/master/Cluster-Layout.md) or tree (https://github.com/d3/d3-3.x-api-reference/blob/master/Tree-Layout.md).

Those both only work for trees; this is a graph. (just realized all of the screenshots so far show trees)

---

Comments from Reviewable

[couchand]

---

Review status: 13 of 27 files reviewed at latest revision, 28 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[BramGruneir]

@vilterp This looks like it's just about ready to merge. Can you squash it into one or two commits? It also looks like you need to rebase against master again.

It also looks like you've got a few more linter errors.

[nvanbenschoten]

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

---

pkg/storage/command_queue.go, line 836 at r34 (raw file):

// children. If it has children, it doesn't add the given command, but
// calls itself on each child. Thus, only leaf commands are added.
func addCommand(commandsSoFar map[int64]storagebase.CommandQueueCommand, command cmd) {

One thing to remember throughout this change is that the storage package's namespace is already really bloated. Any free functions you're adding here will have a package-level visibility, which will be adding to this bloat. Because of this, ambiguous, unqualified names like addCommand are a pretty bad idea. What is a developer going to think when they are writing a test at the Replica level and see addCommand in their autocomplete? I don't want to nit the naming here too harshly, but until #18779 is addressed this is worrying me. Can we do a better job namespacing this, either through qualifying these function names or through a helper type with methods?

Likewise, "snapshot" is an extremely overloaded term at this level, as we already have Raft snapshots. Is there a better name for this that could avoid future confusion.

---

pkg/storage/command_queue_test.go, line 742 at r34 (raw file):

	snapshot := cq.GetSnapshot()
	if n := len(snapshot); n != 2 {

You can pull these length assertions into assertExpectedPrereqs, right?

---

Comments from Reviewable

[BramGruneir]

Reviewed 1 of 12 files at r1, 1 of 12 files at r4, 1 of 3 files at r5, 3 of 12 files at r18, 1 of 2 files at r22, 1 of 3 files at r29, 4 of 18 files at r31, 20 of 20 files at r34.
Review status: all files reviewed at latest revision, 32 unresolved discussions, some commit checks failed.

---

pkg/storage/command_queue.go, line 833 at r34 (raw file):

}

// addCommand adds the given command to the given map, if it has no

This is not very clear. I see what you're trying to say, but it's quite clumsy.

Perhaps start with
addCommand adds all leaf commands to the commandsSoFar map.
And then discuss the tree walk and mention that it will be recursively called.

How large can the tree of commands get? Is recursion the correct choice here?

---

pkg/storage/command_queue.go, line 858 at r34 (raw file):

// filterNonexistentPrereqs removes prereqs which point at commands that
// are no longer in the queue. This can happen e.g. if command C has prereqs

Having both This can happen and e.g. are not correct. Just pick one. But if this is only one case, e.g. would be more correct or perhaps just write For example.

---

pkg/storage/stores.go, line 137 at r34 (raw file):

}

// GetReplicaForRangeID returns the replica which contains the specified range,

In status.go, take a look at Ranges(. We do a store visit there. But we use storage.IterateRangeDescriptors(ctx, store.Engine(). This function might not be needed.

But if you still are going to add this function, it might be nice to replace those many other instances with this. Not in this PR, but as a follow up.

Also, this will return an error if not found. Perhaps that should be mentioned in the description. Or, you could remove the error and rely on the nil alone. I'm not sure what having the error add here. I'm not sure if roachpb.RangeNotFoundError is the only error than can be returned, but if not, you're swallowing the other errors.

Seems to me, if it's the case that the only error could be rangenotfounderror, than the resulting error isn't needed.

Also, please add a test for this.

---

pkg/storage/storagebase/state.proto, line 95 at r34 (raw file):

}

message CommandQueueCommand {

I must say, I'm not of can of the name. Is this used outside of the command queues snapshot? If not, perhaps an embedded message would be better. (and could just be called Command!)

---

pkg/ui/webpack.config.js, line 42 at r26 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Those both only work for trees; this is a graph. (just realized all of the screenshots so far show trees)

This specific change to webpack's config seems like it should exist in it's own pr. How about pulling it out and submitting it own it's own?

---

Comments from Reviewable

[BramGruneir]

pkg/storage/storagebase/state.proto, line 95 at r34 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

I must say, I'm not of can of the name. Is this used outside of the command queues snapshot? If not, perhaps an embedded message would be better. (and could just be called Command!)

Bah, fan of.

---

Comments from Reviewable

[vilterp]

Reviewed 1 of 12 files at r35.
Review status: 17 of 27 files reviewed at latest revision, 32 unresolved discussions, some commit checks failed.

---

pkg/storage/command_queue.go, line 833 at r34 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

This is not very clear. I see what you're trying to say, but it's quite clumsy.

Perhaps start with
addCommand adds all leaf commands to the commandsSoFar map.
And then discuss the tree walk and mention that it will be recursively called.

How large can the tree of commands get? Is recursion the correct choice here?

Done. I think recursion should be fine. Otherwise we would have a single stack frame with a stack data structure, right? Not sure how much that would save us. cc @nvanbenschoten

---

pkg/storage/command_queue.go, line 836 at r34 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

One thing to remember throughout this change is that the storage package's namespace is already really bloated. Any free functions you're adding here will have a package-level visibility, which will be adding to this bloat. Because of this, ambiguous, unqualified names like addCommand are a pretty bad idea. What is a developer going to think when they are writing a test at the Replica level and see addCommand in their autocomplete? I don't want to nit the naming here too harshly, but until #18779 is addressed this is worrying me. Can we do a better job namespacing this, either through qualifying these function names or through a helper type with methods?

Likewise, "snapshot" is an extremely overloaded term at this level, as we already have Raft snapshots. Is there a better name for this that could avoid future confusion.

Done. Defined CommandQueueSnapshot helper type alias, and put these functions under it.

---

pkg/storage/command_queue.go, line 858 at r34 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Having both This can happen and e.g. are not correct. Just pick one. But if this is only one case, e.g. would be more correct or perhaps just write For example.

Done.

---

pkg/storage/command_queue_test.go, line 742 at r34 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

You can pull these length assertions into assertExpectedPrereqs, right?

Done.

---

pkg/storage/stores.go, line 137 at r34 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

In status.go, take a look at Ranges(. We do a store visit there. But we use storage.IterateRangeDescriptors(ctx, store.Engine(). This function might not be needed.

But if you still are going to add this function, it might be nice to replace those many other instances with this. Not in this PR, but as a follow up.

Also, this will return an error if not found. Perhaps that should be mentioned in the description. Or, you could remove the error and rely on the nil alone. I'm not sure what having the error add here. I'm not sure if roachpb.RangeNotFoundError is the only error than can be returned, but if not, you're swallowing the other errors.

Seems to me, if it's the case that the only error could be rangenotfounderror, than the resulting error isn't needed.

Also, please add a test for this.

Done.

---

pkg/storage/storagebase/state.proto, line 95 at r34 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Bah, fan of.

Done.

---

pkg/ui/webpack.config.js, line 42 at r26 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

This specific change to webpack's config seems like it should exist in it's own pr. How about pulling it out and submitting it own it's own?

Done. Needed it for dagre to find Lodash 3, but I just switched to dagre-layout (newer fork) which can use Lodash 4. Going to move this change to separate PR.

---

Comments from Reviewable

[BramGruneir]

Just get @nvanbenschoten to give it a green light.

---

Reviewed 1 of 3 files at r6, 12 of 12 files at r35.
Review status: all files reviewed at latest revision, 30 unresolved discussions, some commit checks failed.

---

pkg/storage/stores.go, line 137 at r34 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Done.

I'm guessing, from this, you've only added a test and fixed the implementation?

---

pkg/storage/storagebase/state.proto, line 110 at r35 (raw file):

  // Timestamp in nanoseconds when this snapshot was taken.
  util.hlc.Timestamp timestamp = 1 [(gogoproto.nullable) = false];
  map<int64, Command> localScope = 2 [(gogoproto.nullable) = false];

What is the int64 in this case? The command id? Would be nice to have a comment here.

Also, just as fair warning, int64 keys for maps is slightly annoying to work with on the javascript side, as you have to convert them into a Long.

---

Comments from Reviewable

[nvanbenschoten]

really nice job on this!

Just remember to squash again before merging.

---

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

---

Comments from Reviewable