ui: add data distribution debug page (aka replica matrix)

[vilterp]

Split off from #20500, which had some CR comments from four months ago. Many have been addressed, but need to comb through again.

Note: if one of its API calls returns an error, it will display the spinner forever. Sadly, the cluster viz has the same problem (as well as many other pages, probably). Kind of want to address this with some changes to Loader as part of #24011, but maybe should just address it here.

[cockroach-teamcity]

This change is 

[vilterp]

note to self: do this (here and on backend) before merging. "data distribution" is a more external-friendly name, so we should prob use that.

[vilterp]

@couchand I think the other TODOs here will shake out as this page is refined & refactored (which I think it will be, since a lot of data could be layered into it). Would prefer to get it in sooner, so we can start getting feedback.

[couchand]

Reviewed 6 of 11 files at r1.
Review status: 6 of 11 files reviewed at latest revision, 1 unresolved discussion.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 57 at r1 (raw file):

  render() {
    // TODO(vilterp): use locality tree selector

Seems like a good idea to do this, rather than adding a second way to do mostly the same thing... For one thing, that already correctly excludes decommissioned nodes.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 74 at r1 (raw file):

    };

    function getValue(dbPath: TreePath, nodePath: TreePath): number {

please move this to a method to provide prop equality

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 83 at r1 (raw file):

        return 0;
      }
      return res.toInt();

need fixLong here?

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 87 at r1 (raw file):

    return (
      <table>

please don't abuse tables for layout

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 94 at r1 (raw file):

                Zone Configs{" "}
                <a href={ZONE_CONFIGS_DOCS_URL}>
                  <small>(?)</small>

perhaps a word or two here, or use the info tooltip pattern?

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 101 at r1 (raw file):

            <td style={{verticalAlign: "top"}}>
              <ReplicaMatrix
                label={<em># Replicas</em>}

please style rather than abusing em

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 108 at r1 (raw file):

                )}
                colLeafLabel={(node, path, isPlaceholder) => (
                  isPlaceholder

please not nested, multi-line ternaries

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 110 at r1 (raw file):

                  isPlaceholder
                    ? ""
                    : node === null

when would this case come up?

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 114 at r1 (raw file):

                    : `n${node.node_id.toString()}`
                )}
                rowNodeLabel={(row: TableDesc) => (`DB: ${row.dbName}`)}

is the "DB" adding value here?

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 133 at r1 (raw file):

  }

  componentDidUpdate() {

I don't think that's the lifecycle method you're looking for.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 142 at r1 (raw file):

      <div>
        <section className="section parent-link">
          <Link to="/debug">&lt; Back to Debug</Link>

i would prefer not

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 145 at r1 (raw file):

        </section>
        <section className="section">
          <h1>Data Distribution</h1>

I think this header should just be in the section below

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 157 at r1 (raw file):

              refreshNodes={this.props.refreshNodes}
              replicaMatrix={this.props.replicaMatrix}
              refreshReplicaMatrix={this.props.refreshReplicaMatrix}

do the referesh functions need to be passed down?

---

pkg/ui/src/views/cluster/containers/dataDistribution/matrix.tsx, line 27 at r1 (raw file):

  rowNodeLabel?: (row: R, path?: TreePath) => string;
  colLeafLabel?: (col: C, path: TreePath, isPlaceholder: boolean) => string;
  colNodeLabel?: (col: C, path: TreePath, isPlaceholder: boolean) => string;

Kind of disappointed with TS that making these properties optional compiles... as far as I can tell not passing them in it would explode.

---

pkg/ui/src/views/cluster/containers/dataDistribution/matrix.tsx, line 30 at r1 (raw file):

}

class Matrix<R, C> extends Component<MatrixProps<R, C>, MatrixState> {

I'd recommend removing most/all of the generalization here. It's making things overly complicated, there seem to be some latent bugs, and there's not a compelling reason to generalize since the replica matrix data distribution page is the only usage of it. Better to make a focused implementation just solving your immediate problems, and when/if additional usage comes up, look for opportunities to abstract then.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 15 at r1 (raw file):

}

export interface LayoutNode<T> {

This file seems closely coupled with matrix.tsx, so I'll reserve detailed comments until after discussion about what to do do about the generalization there.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 45 at r1 (raw file):

 * For instance, the tree
 *
 * (a [(b [c d]) e])

I'm not sure this example is very clear. It would seem to indicate that e needs to be a leaf node, so it is pushed down to the same level as c and d, but I don't understand why the same transformation wouldn't be made to a and b? I think it's just that the notation here is ambiguous, the expected result makes enough sense. I'd suggest drawing the tree out rather than trying to make this compact syntax work, I'm not sure there's a great way to make it unambiguous.

---

pkg/ui/src/views/cluster/containers/dataDistribution/zoneConfigList.tsx, line 18 at r1 (raw file):

class ZoneConfigList extends React.Component<ZoneConfigListProps, {}> {
  componentDidMount() {
    this.props.refreshReplicaMatrix();

Isn't this inside another component already doing this?

---

pkg/ui/src/views/cluster/containers/dataDistribution/zoneConfigList.tsx, line 55 at r1 (raw file):

  (state: AdminUIState) => {
    return {
      zoneConfigs: state.cachedData.replicaMatrix.data

pass in as a prop, since the parent already has it

---

Comments from Reviewable

[vilterp]

Responded to most comments, besides de-generifying the TreeMatrix widget. I agree that there is some wonky code (esp the label functions) that could probably be simplified this way; will hopefully get to it later this week.

---

Review status: 3 of 10 files reviewed at latest revision, 20 unresolved discussions.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 74 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

please move this to a method to provide prop equality

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 83 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

need fixLong here?

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 87 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

please don't abuse tables for layout

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 94 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

perhaps a word or two here, or use the info tooltip pattern?

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 101 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

please style rather than abusing em

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 114 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

is the "DB" adding value here?

Not sure. I'm afraid users might not know what they're looking at without something saying "this is a database" or "this is your schema" or "this is a table". There's probably a better way.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 133 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

I don't think that's the lifecycle method you're looking for.

Done. Guessing componentWillReceiveProps is the one I'm looking for… Hmm don't love this pattern… See #18741 😛

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 142 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

i would prefer not

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 145 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

I think this header should just be in the section below

Hm, tried this but the vertical padding comes from the <section>s, and I don't really want to add it to the h1. All of our main pages have <h1>s in their own <section>s; would prefer to revisit this if/when this gets pulled up as a main page.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 157 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

do the referesh functions need to be passed down?

Nope! Good catch.

---

pkg/ui/src/views/cluster/containers/dataDistribution/matrix.tsx, line 27 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

Kind of disappointed with TS that making these properties optional compiles... as far as I can tell not passing them in it would explode.

Hmm yeah… Made non-optional

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 45 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

I'm not sure this example is very clear. It would seem to indicate that e needs to be a leaf node, so it is pushed down to the same level as c and d, but I don't understand why the same transformation wouldn't be made to a and b? I think it's just that the notation here is ambiguous, the expected result makes enough sense. I'd suggest drawing the tree out rather than trying to make this compact syntax work, I'm not sure there's a great way to make it unambiguous.

a and b can't be pushed down because they have children.

The notation isn't ambiguous (node ::= ident | "(" ident "[" node* "]" ")") but it is pretty durn hard to read. Replaced it with drawn-out trees.

---

pkg/ui/src/views/cluster/containers/dataDistribution/zoneConfigList.tsx, line 55 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

pass in as a prop, since the parent already has it

Moved this back into the DataDistribution component. Without all the boilerplate it's only 23 lines.

---

Comments from Reviewable

[vilterp]

Largely de-generified the Matrix component. Both rows and columns still use a generic TreeNode structure, though. PTAL.

---

Review status: 0 of 15 files reviewed at latest revision, 20 unresolved discussions, all commit checks successful.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 57 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

Seems like a good idea to do this, rather than adding a second way to do mostly the same thing... For one thing, that already correctly excludes decommissioned nodes.

Uses the locality tree selector now.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 108 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

please not nested, multi-line ternaries

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 110 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

when would this case come up?

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/matrix.tsx, line 30 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

I'd recommend removing most/all of the generalization here. It's making things overly complicated, there seem to be some latent bugs, and there's not a compelling reason to generalize since the replica matrix data distribution page is the only usage of it. Better to make a focused implementation just solving your immediate problems, and when/if additional usage comes up, look for opportunities to abstract then.

Done. (at least most of the way)

---

Comments from Reviewable

[couchand]

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

---

pkg/ui/src/index.tsx, line 145 at r3 (raw file):

          <Route path="nodes" component={ Nodes } />
          <Route path="settings" component={ Settings } />
          <Route path="datadistribution" component={ DataDistributionPage } />

this URL slug seems kind of stilted but it looks like it's consistent with other routes so it's fine.

---

pkg/ui/src/redux/apiReducers.ts, line 246 at r3 (raw file):

  api.getDataDistribution,
  "dataDistribution",
  moment.duration(1, "m"),

Just wanted to check if this is the right refresh interval?

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 114 at r1 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Not sure. I'm afraid users might not know what they're looking at without something saying "this is a database" or "this is your schema" or "this is a table". There's probably a better way.

random idea: the databases icon... is that crazy? @josueeee will kill me

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 39 at r3 (raw file):

    const zoneConfigs = this.props.dataDistribution.zone_configs;
    const sortedIDs = Object.keys(zoneConfigs);
    sortedIDs.sort();

this is kind of expensive for a render method, maybe use a selector?

---

pkg/ui/src/views/cluster/containers/dataDistribution/replicaMatrix.styl, line 21 at r3 (raw file):

    font-style italic

  &__row--node

node seems ambiguous to be used like this. Maybe something more explicit like collapsible?

---

pkg/ui/src/views/cluster/containers/dataDistribution/replicaMatrix.styl, line 27 at r3 (raw file):

    background-color rgb(240, 240, 240)

  &__row-label

maybe header to keep it consistent with the columns?

---

pkg/ui/src/views/cluster/containers/dataDistribution/replicaMatrix.tsx, line 40 at r3 (raw file):

    super(props);
    this.state = {
      collapsedRows: [["system"]],

maybe want to add the other two default databases (though they're empty by default so maybe it's fine?). maybe sort them to the end?

---

pkg/ui/src/views/cluster/containers/dataDistribution/replicaMatrix.tsx, line 69 at r3 (raw file):

  }

  colLabel(col: LayoutNode<NodeDescriptor$Properties>, colIsCollapsed: boolean): string {

what's the difference between colIsCollapsed and col.isCollapsed?

---

pkg/ui/src/views/cluster/containers/dataDistribution/replicaMatrix.tsx, line 143 at r3 (raw file):

            return (
              <tr
                key={JSON.stringify(row)}

that's a pretty heavy key

---

pkg/ui/src/views/cluster/containers/dataDistribution/replicaMatrix.tsx, line 156 at r3 (raw file):

                    { "matrix__row-label--node": !row.isLeaf },
                  )}
                  style={{ paddingLeft: row.depth * ROW_TREE_INDENT_PX + 5 }}

what's the + 5?

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 17 at r3 (raw file):

export interface LayoutNode<T> {
  width: number;
  depth: number; // Depth of this subtree. Leaves have a depth of 1.

I believe the terminology here is a bit off: depth is the number of edges between a node and the root node, and height is the maximum number of edges between a node and a descendant leaf node. So this field is actually storing height + 1.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 93 at r3 (raw file):

export function layoutTree<T>(root: TreeNode<T>, collapsedPaths: TreePath[]): LayoutNode<T>[][] {
  const depth = getDepth(root);
  function recur(node: TreeNode<T>, pathToThis: TreePath): LayoutNode<T>[][] {

I think coming up with a real name for this would be a good exercise and would clarify the intention.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 95 at r3 (raw file):

  function recur(node: TreeNode<T>, pathToThis: TreePath): LayoutNode<T>[][] {
    const depthUnderThis = depth - pathToThis.length;
    const placeholderRows = _.range(depthUnderThis).reverse().map((thisDepth) => (

it's not entirely satisfying that we do this whether or not we actually need the rows...

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 99 at r3 (raw file):

        {
          width: 1,
          depth: thisDepth + 1,

I don't think this can be correct for both the cases it's used below... shouldn't one of them be +1? the leaf case I think?

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 144 at r3 (raw file):

      recur(childNode, [...pathToThis, childNode.name])
    ));
    const maxDepth = _.maxBy(childLayouts, (cl) => cl[0][0].depth)[0][0].depth;

just map > max if all you need is the value.

also, what is this wonky access? oh, i guess it's to get the root node's depth... hmmm

also, shouldn't they all be the same anyway because of the placeholders?

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 145 at r3 (raw file):

    ));
    const maxDepth = _.maxBy(childLayouts, (cl) => cl[0][0].depth)[0][0].depth;
    const transposedChildLayouts = _.range(depth).map(() => ([]));

It's not clear to me why this is depth and not depth - 1, since you're adding the top row down below...

Also, I don't think this is transposing anything?

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 150 at r3 (raw file):

      _.forEach(childLayout, (row, rowIdx) => {
        _.forEach(row, (col) => {
          transposedChildLayouts[rowIdx].push(col);

maybe this is too clever, but

const laidOutChildren = _.map(_.zip(childLayouts), _.concat);

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 170 at r3 (raw file):

  }

  const recurRes = recur(root, []);

splitting the body of the function around the nested closure makes seeing the whole thing harder

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 174 at r3 (raw file):

}

function removePlaceholdersFromEnd<T>(arr: LayoutNode<T>[][]): LayoutNode<T>[][] {

suggest naming this removePlaceholdersFromBottom

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 183 at r3 (raw file):

    output.push(row);
  }
  return output;

maybe use _.takeWhile?

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 246 at r3 (raw file):

  tree: TreeNode<T>,
  collapsedPaths: TreePath[],
  includeNodes: boolean,

I'd call this onlyLeaves or includeInternalNodes... I think leaf nodes are still nodes

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 249 at r3 (raw file):

): FlattenedNode<T>[] {
  const output: FlattenedNode<T>[] = [];
  function recur(node: TreeNode<T>, depth: number, pathSoFar: TreePath) {

I think giving this function a real name would be valuable, and I'd prefer if the body of the main function weren't split around it.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 259 at r3 (raw file):

      });
    } else {
      const isExpanded = _.filter(collapsedPaths, (p) => _.isEqual(p, pathSoFar)).length === 0;

_.some maybe? or deepIncludes?

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 260 at r3 (raw file):

    } else {
      const isExpanded = _.filter(collapsedPaths, (p) => _.isEqual(p, pathSoFar)).length === 0;
      if (includeNodes || !isExpanded) {

it took me quite a while to figure out the logic here. The || !isExpanded part is to include a single placeholder instead of all the leaves that are being hidden. Collapsing an internal node effectively makes it the leaf node.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 291 at r3 (raw file):

  const child = root.children.find((c) => (c.name === pathSegment));
  if (child === undefined) {
    throw new Error(`not found: ${path}`);

maybe this error should provide more context? do you ever catch it?

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 366 at r3 (raw file):

 * ]
 */
function cartProd<A, B>(as: A[], bs: B[]): {a: A, b: B}[] {

Doesn't seem like two nested loops deserve a function, but... Sure, I guess.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 395 at r3 (raw file):

 *   colTree = (C_1 [C_2 C_3])
 *
 * calling sumValuesUnderPath(rowTree, colTree, ['R_a'], ['C_b'], getValue)

s/C_b/C_1/ here and below

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.spec.ts, line 123 at r3 (raw file):

      // |  b  |  e  |
      // TODO(vilterp): these depth numbers don't reflect that anything was collapsed.
      // maybe that's ok; doesn't mess up the rendering.

what are the depth numbers used for? if they're not broken then the todo's not needed here; if they aren't used the property isn't needed.

---

Comments from Reviewable

[vilterp]

Review status: complete! 0 of 0 LGTMs obtained

---

pkg/ui/src/index.tsx, line 145 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

this URL slug seems kind of stilted but it looks like it's consistent with other routes so it's fine.

data-distribution?

---

pkg/ui/src/redux/apiReducers.ts, line 246 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

Just wanted to check if this is the right refresh interval?

Was just copying a bunch of others that are 1m. It seems reasonable to me.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 114 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

random idea: the databases icon... is that crazy? @josueeee will kill me

I actually had the same thought… Having icons in the schema object tree is actually par for the course in DB admin tools screenshots, and a decent solution, although it does clutter things…

---

pkg/ui/src/views/cluster/containers/dataDistribution/replicaMatrix.tsx, line 69 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

what's the difference between colIsCollapsed and col.isCollapsed?

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 17 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

I believe the terminology here is a bit off: depth is the number of edges between a node and the root node, and height is the maximum number of edges between a node and a descendant leaf node. So this field is actually storing height + 1.

Done. (replaced depth with isLeaf)

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 99 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

I don't think this can be correct for both the cases it's used below... shouldn't one of them be +1? the leaf case I think?

Done (removed depth)

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.spec.ts, line 123 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

what are the depth numbers used for? if they're not broken then the todo's not needed here; if they aren't used the property isn't needed.

Done.

---

Comments from Reviewable

[couchand]

Reviewed 2 of 12 files at r4, 11 of 11 files at r5.
Review status: complete! 0 of 0 LGTMs obtained

---

pkg/ui/src/index.tsx, line 145 at r3 (raw file):

Previously, vilterp (Pete Vilter) wrote…

data-distribution?

i like sausage case but i also like consistency... I guess it's only problemranges that's currently smash-case, so yeah, let's use sausage

---

pkg/ui/src/util/docs.ts, line 15 at r5 (raw file):

export const cancelJob = docsURL("cancel-job.html");
export const enableNodeMap = docsURL("enable-node-map.html");
export const zoneConfigs = docsURL("configure-replication-zones.html");

I tried to make these just the names of the pages (I guess startFlags breaks that pattern...)

---

Comments from Reviewable

[vilterp]

Addressed everything except comments on layoutTree, the gnarliest function… Diving into that next.

---

Review status: complete! 0 of 0 LGTMs obtained

---

pkg/ui/src/index.tsx, line 145 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

i like sausage case but i also like consistency... I guess it's only problemranges that's currently smash-case, so yeah, let's use sausage

Done.

---

pkg/ui/src/util/docs.ts, line 15 at r5 (raw file):

Previously, couchand (Andrew Couch) wrote…

I tried to make these just the names of the pages (I guess startFlags breaks that pattern...)

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/index.tsx, line 39 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

this is kind of expensive for a render method, maybe use a selector?

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/replicaMatrix.styl, line 21 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

node seems ambiguous to be used like this. Maybe something more explicit like collapsible?

Done. (Went with internal-node)

---

pkg/ui/src/views/cluster/containers/dataDistribution/replicaMatrix.styl, line 27 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

maybe header to keep it consistent with the columns?

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/replicaMatrix.tsx, line 40 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

maybe want to add the other two default databases (though they're empty by default so maybe it's fine?). maybe sort them to the end?

crdb_internal and information_schema? They never show up here — probably because all their tables are virtual and thus never show up in the meta ranges which power this endpoint.

---

pkg/ui/src/views/cluster/containers/dataDistribution/replicaMatrix.tsx, line 143 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

that's a pretty heavy key

Done (row.path.join("/") instead of JSON.stringify(row). Don't know how to identify a row by anything lighter-weight than its path.

---

pkg/ui/src/views/cluster/containers/dataDistribution/replicaMatrix.tsx, line 156 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

what's the + 5?

Factored out into a constant. Sadly you can't add margins to elements, so I'm adding it to the padding.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 246 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

I'd call this onlyLeaves or includeInternalNodes... I think leaf nodes are still nodes

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 249 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

I think giving this function a real name would be valuable, and I'd prefer if the body of the main function weren't split around it.

Me trying to come up with a meaningful name: https://twitter.com/sallar/status/997459417242140672

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 259 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

_.some maybe? or deepIncludes?

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 260 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

it took me quite a while to figure out the logic here. The || !isExpanded part is to include a single placeholder instead of all the leaves that are being hidden. Collapsing an internal node effectively makes it the leaf node.

Yes. Broke out another variable to hopefully make things more clear.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 291 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

maybe this error should provide more context? do you ever catch it?

Nope, never catch it :/ Relying on tests for its callers (sumValuesUnderPaths and layoutTree) to make sure it never gets thrown.

Not sure what context to add that wouldn't be in a stack trace.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 395 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

s/C_b/C_1/ here and below

Done.

---

Comments from Reviewable

[vilterp]

Decomposed the layoutTree stuff a bit. I think it's significantly less gnarly now. Could go farther (like maybe making Layout a class), but I think we're reaching a point of diminishing returns.

---

Review status: complete! 0 of 0 LGTMs obtained

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 93 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

I think coming up with a real name for this would be a good exercise and would clarify the intention.

Me trying to come up with a name: https://twitter.com/sallar/status/997459417242140672

I don't know how important this is. If I had written it iteratively, there would be a while loop with a stack called treeNodeStackor some other non-name.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 95 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

it's not entirely satisfying that we do this whether or not we actually need the rows...

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 145 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

It's not clear to me why this is depth and not depth - 1, since you're adding the top row down below...

Also, I don't think this is transposing anything?

Done. True, it was horizontally concatenating, not transposing. Factored out a function with that name.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 150 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

maybe this is too clever, but

const laidOutChildren = _.map(_.zip(childLayouts), _.concat);

Huh, idk. Does seem a little too clever; might just stick with the good ol' nested loops.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 170 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

splitting the body of the function around the nested closure makes seeing the whole thing harder

If I make it a top-level function instead of a closure, I have to pass parameters… Can do this if it's important to you, but it doesn't seem important to me.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 174 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

suggest naming this removePlaceholdersFromBottom

Done. (removed this function)

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 183 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

maybe use _.takeWhile?

Done. (removed this function)

---

Comments from Reviewable

[couchand]

---

Reviewed 5 of 16 files at r6, 8 of 11 files at r7, 3 of 3 files at r8.
Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/ui/src/views/cluster/containers/dataDistribution/replicaMatrix.tsx, line 40 at r3 (raw file):

Previously, vilterp (Pete Vilter) wrote…

crdb_internal and information_schema? They never show up here — probably because all their tables are virtual and thus never show up in the meta ranges which power this endpoint.

defaultdb and postgres

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 170 at r3 (raw file):

Previously, vilterp (Pete Vilter) wrote…

If I make it a top-level function instead of a closure, I have to pass parameters… Can do this if it's important to you, but it doesn't seem important to me.

I just mean move the return ... statement above the closure definition and rely on hoisting

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 291 at r3 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Nope, never catch it :/ Relying on tests for its callers (sumValuesUnderPaths and layoutTree) to make sure it never gets thrown.

Not sure what context to add that wouldn't be in a stack trace.

A lot of the time we don't get a stack trace, we just get the message.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 195 at r8 (raw file):

    _.forEach(childLayout, (row, rowIdx) => {
      _.forEach(row, (col) => {
        output[rowIdx].push(col);

I still want this to use zip

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 205 at r8 (raw file):

/**
 * verticalConcatLayouts takes an array of layouts and returns
 * a new layout composed of its inptus laid out vertically.

s/inptus/inputs/

---

Comments from Reviewable

[vilterp]

Thank Jesus, it's finally going in! Original commit that I've been squashing into dated 12/4/17 😂

bors r+

---

Review status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/ui/src/views/cluster/containers/dataDistribution/replicaMatrix.tsx, line 40 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

defaultdb and postgres

Done.

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 170 at r3 (raw file):

Previously, couchand (Andrew Couch) wrote…

I just mean move the return ... statement above the closure definition and rely on hoisting

Done. Nice, didn't know you could do that!

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 195 at r8 (raw file):

Previously, couchand (Andrew Couch) wrote…

I still want this to use zip

Tried, but there was a type error I didn't feel like figuring out…

---

pkg/ui/src/views/cluster/containers/dataDistribution/tree.ts, line 205 at r8 (raw file):

Previously, couchand (Andrew Couch) wrote…

s/inptus/inputs/

Done.

---

Comments from Reviewable

[craig]

Build succeeded

• GitHub CI (Cockroach)