config: convert ZoneConfig to the new format

[d4l3k]

Converts ZoneConfig to the new format specified in the expressive ZoneConfig
RFC. This is intermediate work on #4868.
https://github.com/cockroachdb/cockroach/blob/develop/docs/RFCS/expressive_zone_config.md

• Allocator and StorePool use []config.Constraint instead of roachpb.Attributes
  since the new type allows for different types of constraint.
• Adds a ZoneConfigLegacy struct for upgrades to the new format.
• Adds a ZoneConfigHuman struct for easier specification on the command line.

@petermattis @bdarnell

---

This change is

[spencerkimball]

s/constrains/constraints/

[d4l3k]

Review status: 0 of 25 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

config/config.proto, line 69 [r1] (raw file):

Previously, spencerkimball (Spencer Kimball) wrote…

s/constrains/constraints/

Done.

---

Comments from Reviewable

[bdarnell]

Reviewed 18 of 25 files at r1, 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

config/config.proto, line 60 [r2] (raw file):

// ZoneConfig holds configuration that is needed for a range of KV pairs. This
// and the conversion methods must stay in sync with ZoneConfigHuman.
message ZoneConfig {

Add reserved 1 to both message types so we won't try to reuse the deleted tag 1.

---

config/config.proto, line 74 [r2] (raw file):

}

// ZoneConfigHuman is the human readable shorthand form of ZoneConfig. This and

A protobuf seems like it's only halfway human-readable, and it looks like we never actually use the proto-ness of ZoneConfigHuman. Should it be a plain struct instead (or make To/FromHuman go all the way to strings)? Or could we put MarshalYAML and UnmarshalYAML methods on the Constraint type to get yaml parsing to work the way we want?

---

config/config.proto, line 86 [r2] (raw file):

// ZoneConfigLegacy is the old version of ZoneConfig and is used for upgrades.
// TODO(d4l3k): Remove after a sufficient amount of time has passed.
message ZoneConfigLegacy {

Shouldn't this also have the range_{min,max}_bytes and gc fields? Could we leave the replica_attrs field in ZoneConfig and do away with the extra proto?

---

Comments from Reviewable

[d4l3k]

Review status: 0 of 27 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

---

config/config.proto, line 60 [r2] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Add reserved 1 to both message types so we won't try to reuse the deleted tag 1.

Done.

---

config/config.proto, line 74 [r2] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

A protobuf seems like it's only halfway human-readable, and it looks like we never actually use the proto-ness of ZoneConfigHuman. Should it be a plain struct instead (or make To/FromHuman go all the way to strings)? Or could we put MarshalYAML and UnmarshalYAML methods on the Constraint type to get yaml parsing to work the way we want?

Completely slipped my mind that we could use MarshalYAML and UnmarshalYAML. Removed all the ZoneConfigHuman stuff.

---

config/config.proto, line 86 [r2] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Shouldn't this also have the range_{min,max}_bytes and gc fields? Could we leave the replica_attrs field in ZoneConfig and do away with the extra proto?

We could leave the `replica_attrs` field in `ZoneConfig` but I feel like having it as a separate proto makes it less confusing. Having a field on a proto that's never supposed to be used sounds hazardous.

As for the other fields, the config.MigrateZoneConfig merges the old and the new proto when upgrading.

---

Comments from Reviewable

[bdarnell]

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

---

config/config.proto, line 86 [r2] (raw file):

Previously, d4l3k (Tristan Rice) wrote…

We could leave the replica_attrs field in ZoneConfig but I feel like having it as a separate proto makes it less confusing. Having a field on a proto that's never supposed to be used sounds hazardous.

As for the other fields, the config.MigrateZoneConfig merges the old and the new proto when upgrading.

But that's the usual case for protobuf evolution: the old fields stick around for as long as they are supported. Removing the field from the "real" protobuf and moving it to a separate "legacy" protobuf is weird. I'd prefer to keep this field in the regular ZoneConfig proto unless there's something specific (like yaml encoding) that makes this tricky to do cleanly.

---

Comments from Reviewable

[d4l3k]

Review status: 0 of 27 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

---

config/config.proto, line 86 [r2] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

But that's the usual case for protobuf evolution: the old fields stick around for as long as they are supported. Removing the field from the "real" protobuf and moving it to a separate "legacy" protobuf is weird. I'd prefer to keep this field in the regular ZoneConfig proto unless there's something specific (like yaml encoding) that makes this tricky to do cleanly.

Done.

---

Comments from Reviewable

[d4l3k]

@bdarnell boop

[bdarnell]

---

Reviewed 2 of 27 files at r3, 25 of 25 files at r4.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable