cli: make the zone cli aware of tables/databases

[petermattis]

Enhanced the zone commands to operate on database and table names
instead of object IDs.

Fixes #2505.

[petermattis]

This likely needs some additional commenting, but I wanted to get some feedback on the interface. Specifying zone configs via yaml isn't that pleasant, but I think this is as much as we should do for Beta.

[mberhault]

I agree with the format, but yaml is still a friendlier solution than json (kindda gross) or ascii protobufs (too obscure). an alternative would be to allow setting individual fields of the config, but I'm not sure that's critical.

[tamird]

Lots of utility stuff wound up in zone.go - probably belongs elsewhere. It makes the important stuff in zone.go quite hard to read in this diff.

---

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

---

cli/zone.go, line 54 [r1] (raw file):
mixing make and literal =/

also, why pointers?

---

cli/zone.go, line 64 [r1] (raw file):
since you're doing a checked type assertion in unmarshal proto, maybe check here too

---

cli/zone.go, line 294 [r1] (raw file):
"referred to be"?

---

sql/keys.go, line 59 [r1] (raw file):
why not rename the existing method?

---

Comments from the review on Reviewable.io

[petermattis]

@mberhault This change does allow you to tweak individual fields. See the example where range_max_bytes is changed. What I still find unsatisfying is the replica specification. Often we'll want to just change the number of replicas. This is currently accomplished via:

cockroach zone set system "replicas:
- attrs: []
- attrs: []
- attrs: []
- attrs: []
...
"

It would be nice to have a shorthand: cockroach zone set system r=5. Or something like that.

[petermattis]

Review status: 3 of 8 files reviewed at latest revision, 4 unresolved discussions.

---

cli/zone.go, line 54 [r1] (raw file):
Pointers because we're parsing pointers. Doesn't particularly matter one way or the other.

---

cli/zone.go, line 64 [r1] (raw file):
Done.

---

cli/zone.go, line 294 [r1] (raw file):
referred to by

---

sql/keys.go, line 59 [r1] (raw file):
I intended to and just forgot. Done.

---

Comments from the review on Reviewable.io

[mberhault]

can you add a case to modify .default. it looks fine since it should always exist so the UPDATE vs INSERT check should always say UPDATE, but the use of 0 as a return value for queryZonePath makes it worth checking.

[petermattis]

Done.

[mberhault]

LGTM

[bdarnell]

Instead of shorthand at the CLI level, we should consider reworking the protobuf completely. In addition to making it awkward to express the basic constraint of the number of target replicas, it is completely unable to express negative constraints (any store but hdd) or diversity constraints (in a deployment with more datacenters than the replication factor, we don't want two in the same DC, but we can't get that today without pinning the specific datacenters).

I'd be in favor of introducing a num_replicas field which would be used in place of len(attrs). If attrs are present they would be used to constrain the replica selection in the way that they are today, otherwise it is as if there are num_replicas empty attr entries. No need to actually address the issues around negative or diversity constraints at this point.

[petermattis]

@bdarnell That's a good point about negative and diversity constraints. I think we need to put additional thought into this. I'll file an issue.

[tamird]

Reviewed 5 of 5 files at r2.
Review status: 7 of 8 files reviewed at latest revision, 2 unresolved discussions.

---

cli/zone.go, line 54 [r1] (raw file):
I'd prefer values because there's no need to permit nil here.

---

Comments from the review on Reviewable.io

[petermattis]

Review status: 6 of 8 files reviewed at latest revision, 2 unresolved discussions.

---

cli/zone.go, line 54 [r1] (raw file):
Done.

---

Comments from the review on Reviewable.io

[tamird]

Reviewed 1 of 1 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[petermattis]

@jseldess The zone commands are ready for documentation.