ui: added node constraint solver to UI

[d4l3k]

This builds on the rest of the allocator PRs (#8786). Only review the last commit in this PR.

cc @maxlang @BramGruneir

---

This change is

[maxlang]

---

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

---

ui/app/containers/nodeConstraints.tsx, line 20 [r8] (raw file):

interface ConstraintsData {
  state: KeyedCachedDataReducerState<cockroach.server.serverpb.ConstraintsResponseMessage>;

State is a pretty overloaded word, since we already have the Redux state and the React component state.

---

ui/app/containers/nodeConstraints.tsx, line 32 [r8] (raw file):

interface ConstraintsState {
  constraints: string;

I feel like this could be hard to follow, since this isn't actually a list of constraints but more of a constraint selector of some sort?

---

ui/app/containers/nodeConstraints.tsx, line 39 [r8] (raw file):

 * Constraints component.
 */
type ConstraintsProps = ConstraintsData & ConstraintsActions;

Since the types here a pretty simple, you can just combine everything straight up, without composing it piecewise.

---

ui/app/containers/nodeConstraints.tsx, line 59 [r8] (raw file):

  componentDidMount() {
    this.refreshConstraints();
    const node = ReactDom.findDOMNode(this);

@mrtracy Do you prefer using ReactDom.findDOMNode(this); or the ref technique you used in linegraph.tsx?

---

ui/app/containers/nodeConstraints.tsx, line 61 [r8] (raw file):

    const node = ReactDom.findDOMNode(this);
    this.chart = d3.select(node).select("svg");
    setTimeout(this.updateChart.bind(this), 1);

Why do you need to wait for the next tick here? Also you probably don't need to specify an actual delay. You can leave off that parameter.

---

ui/app/containers/nodeConstraints.tsx, line 77 [r8] (raw file):

    for (let candidate of state.data.candidates) {
      candidates[candidate.desc.store_id] = candidate;
    }

Some of this can be rewritten nicely with lodash.

const candidates = _.keyBy(state.data.candidates, "desc.store_id")

or, to maintain type safety,

const candidates = _.keyBy(state.data.candidates, (c)=>c.desc.store_id)

---

ui/app/containers/nodeConstraints.tsx, line 79 [r8] (raw file):

    }

    const nodes: {[id: number]: cockroach.roachpb.StoreDescriptor[]} = {};

nodes = _.groupBy(state.state.stores, (s) => s.node.node_id)

---

ui/app/containers/nodeConstraints.tsx, line 91 [r8] (raw file):

    const datums: {[name: string]: Datum} = {};
    const root: Datum = {name: "Cluster", children: []};
    _.each(nodes, (stores: cockroach.roachpb.StoreDescriptor[], id: string) => {

I feel like this big _.each should be split into two parts. First map across each node, creating a node with name, attrs, children and tiers, then insert each one into the tree, removing tiers if necessary.

---

ui/app/containers/nodeConstraints.tsx, line 92 [r8] (raw file):

    const root: Datum = {name: "Cluster", children: []};
    _.each(nodes, (stores: cockroach.roachpb.StoreDescriptor[], id: string) => {
      const nodeDesc = stores[0].node;

Why are you only ever using stores[0]?

---

ui/app/containers/nodeConstraints.tsx, line 121 [r8] (raw file):

        }
        prefix = tierName;
        container = c;

What about writing this recursively so you aren't setting external values in a foreach callback?

---

ui/app/containers/nodeConstraints.tsx, line 136 [r8] (raw file):

  updateChart() {
    const root = this.structuredStores();

I'm not sure if you borrowed any of this code from a d3 example, but you should attribute it if you did.

---

ui/app/containers/nodeConstraints.tsx, line 174 [r8] (raw file):

    gnode.append("circle")
      .attr("r", 4.5)
      .style("fill", (d: Datum) => d.candidate ? "green" : "black");

Eventually we should update this chart to conform to the styles of the rest of the site.

---

ui/app/util/api.ts, line 200 [r8] (raw file):

export function getConstraints(req: ConstraintsRequestMessage): Promise<ConstraintsResponseMessage> {
  // NB: raftDebug intentionally does not pass a timeout through.

Change comment.

---

Comments from Reviewable

[maxlang]

cc @mrtracy

[maxlang]

Also: looks awesome!

[d4l3k]

Thanks!

---

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

---

ui/app/containers/nodeConstraints.tsx, line 20 [r8] (raw file):

Previously, maxlang (Max Lang) wrote…

State is a pretty overloaded word, since we already have the Redux state and the React component state.

True, but this is literally the KeyedCachedDataReducer_State_.

---

ui/app/containers/nodeConstraints.tsx, line 32 [r8] (raw file):

Previously, maxlang (Max Lang) wrote…

I feel like this could be hard to follow, since this isn't actually a list of constraints but more of a constraint selector of some sort?

It is the constraints, just the shorthand notation for them. It's the same format that is used when specifying ZoneConfig on the command line.

https://github.com/cockroachdb/cockroach/blob/develop/docs/RFCS/expressive_zone_config.md#examples

---

ui/app/containers/nodeConstraints.tsx, line 39 [r8] (raw file):

Previously, maxlang (Max Lang) wrote…

Since the types here a pretty simple, you can just combine everything straight up, without composing it piecewise.

Done.

---

ui/app/containers/nodeConstraints.tsx, line 61 [r8] (raw file):

Previously, maxlang (Max Lang) wrote…

Why do you need to wait for the next tick here? Also you probably don't need to specify an actual delay. You can leave off that parameter.

Done.

---

ui/app/containers/nodeConstraints.tsx, line 77 [r8] (raw file):

Previously, maxlang (Max Lang) wrote…

Some of this can be rewritten nicely with lodash.

const candidates = _.keyBy(state.data.candidates, "desc.store_id")

or, to maintain type safety,

const candidates = _.keyBy(state.data.candidates, (c)=>c.desc.store_id)

Lodash is amazing. Done.

---

ui/app/containers/nodeConstraints.tsx, line 79 [r8] (raw file):

Previously, maxlang (Max Lang) wrote…

nodes = _.groupBy(state.state.stores, (s) => s.node.node_id)

Done.

---

ui/app/containers/nodeConstraints.tsx, line 91 [r8] (raw file):

Previously, maxlang (Max Lang) wrote…

I feel like this big _.each should be split into two parts. First map across each node, creating a node with name, attrs, children and tiers, then insert each one into the tree, removing tiers if necessary.

Done.

---

ui/app/containers/nodeConstraints.tsx, line 92 [r8] (raw file):

Previously, maxlang (Max Lang) wrote…

Why are you only ever using stores[0]?

We use stores in `_.map(stores,` a little bit further down. We're using store[0] here because all stores on a node will have the same node descriptor and we only need to access one of the redundant copies.

---

ui/app/containers/nodeConstraints.tsx, line 121 [r8] (raw file):

Previously, maxlang (Max Lang) wrote…

What about writing this recursively so you aren't setting external values in a foreach callback?

Done with a recursive build and merge.

---

ui/app/containers/nodeConstraints.tsx, line 136 [r8] (raw file):

Previously, maxlang (Max Lang) wrote…

I'm not sure if you borrowed any of this code from a d3 example, but you should attribute it if you did.

Done.

---

ui/app/util/api.ts, line 200 [r8] (raw file):

Previously, maxlang (Max Lang) wrote…

Change comment.

Done.

---

Comments from Reviewable

[mrtracy]

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

---

ui/app/containers/nodeConstraints.tsx, line 59 [r8] (raw file):

Previously, maxlang (Max Lang) wrote…

@mrtracy Do you prefer using ReactDom.findDOMNode(this); or the ref technique you used in linegraph.tsx?

ref is preferred whenever possible https://facebook.github.io/react/docs/more-about-refs.html#a-complete-example

---

Comments from Reviewable

[d4l3k]

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

---

ui/app/containers/nodeConstraints.tsx, line 59 [r8] (raw file):

Previously, mrtracy wrote…

ref is preferred whenever possible https://facebook.github.io/react/docs/more-about-refs.html#a-complete-example

Done.

---

Comments from Reviewable

[maxlang]

---

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

---

ui/app/containers/nodeConstraints.tsx, line 77 [r8] (raw file):

Previously, d4l3k (Tristan Rice) wrote…

Lodash is amazing. Done.

Haha, yes it is.

---

ui/app/containers/nodeConstraints.tsx, line 121 [r8] (raw file):

Previously, d4l3k (Tristan Rice) wrote…

Done with a recursive build and merge.

I like this a lot better.

---

ui/app/containers/nodeConstraints.tsx, line 90 [r9] (raw file):

  mergeDatum(base: Datum, addition: Datum) {
    for (let d of base.children) {
      if (d.name === addition.name) {

This feels like a _.find?

const d = _.find(base.children, {name: addition.name}) or const d = _.find(base.children, (c) => c.name === addition.name)
Then

if (d) { 
  _.each(addition.children, (c)=> this.mergeDatum(d, c)) 
} else {
   base.children.push(addition);
}

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/8887#-:-KQWaO401EiyxRHeDsjL:b-uwkowd)*
<!-- Sent from Reviewable.io -->

[d4l3k]

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

---

ui/app/containers/nodeConstraints.tsx, line 90 [r9] (raw file):

Previously, maxlang (Max Lang) wrote…

This feels like a _.find?

const d = _.find(base.children, {name: addition.name}) or const d = _.find(base.children, (c) => c.name === addition.name)
Then

if (d) { 
  _.each(addition.children, (c)=> this.mergeDatum(d, c)) 
} else {
   base.children.push(addition);
}
</details>
Done.

---

Comments from Reviewable

[d4l3k]

Fixed an out of bounds error and added showing errors to the UI.

[vivekmenezes]

reopen if you still care about this PR

[tamird]

I think we want this. @spencerkimball?

I doubt @d4l3k will finish it up, though.

[spencerkimball]

Cool UI, but I don't believe we need it right now.

[BramGruneir]

@piyush-singh and @vilterp for inspiration.

[vilterp]

Thanks for calling this out @BramGruneir. I see something like this fitting into an end-to-end zone config editing experience integrated into the data distribution page (some discussion of this in #6306 as well):

1. see how your data is distributed now
2. get a preview of how it would be distributed if you changed config in a certain way (what this PR seems to be doing)
3. confirm that you want to make a change
4. see rebalancing progress

This prototype (video) was my idea of how such a UI would work.