server(ccl)/ui: identify zone configs by zone name, not ID

[vilterp]

Previously, the DataDistribution endpoint was returning a map of zone configs by zone config ID. However, zone config ID is not a unique identier within crdb_internal.zones, since subzones attached to partitions share the same ID (that of the top-level zone config), so map entries were being overwritten and the endpoint was returning an incomplete list.

Zone names should be unique; this change uses those instead and updates the UI accordingly.

This change also introduces a serverccl package for the unit test to live in, since testing this code path requires creating partitions, a CCL feature.

Fixes #27718

Release note: None

[cockroach-teamcity]

This change is 

[cockroach-teamcity]

This change is 

[vilterp]

Can reserve this field number and make a new field (instead of just changing the type) if folks think that's important.

[ridwanmsharif]

Looks good to me, I think zone_name is a unique identifier for a zone, however I'm not completely sure. I defer to @knz or @benesch on that.

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/server/admin.go, line 1540 at r1 (raw file):

	zoneConfigsQuery := `
		SELECT zone_name, config_sql, config_protobuf 
    FROM crdb_internal.zones WHERE cli_specifier IS NOT NULL

WHERE zone_name IS NOT NULL

[vilterp]

Hm, linter seems to be mad about just having tests in a package:

go build github.com/cockroachdb/cockroach/pkg/ccl/serverccl: no non-test Go files in /Users/vilterp/go/src/github.com/cockroachdb/cockroach/pkg/ccl/serverccl

May need to move it to partitionccl unless there's some flag to make the linter ok with this; haven't found it yet.

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/server/admin.go, line 1540 at r1 (raw file):

Previously, ridwanmsharif (Ridwan Sharif) wrote…

WHERE zone_name IS NOT NULL

When would it be null?

[benesch]

May need to move it to partitionccl unless there's some flag to make the linter ok with this; haven't found it yet.

Heh, convention is to just create a doc.go that contains nothing but the package name:

// Package serverccl houses tests that verify CCL behavior of a running CockroachDB server.
package serverccl

Reviewed 5 of 6 files at r1, 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

---

pkg/server/admin.go, line 1540 at r1 (raw file):

Previously, vilterp (Pete Vilter) wrote…

When would it be null?

zone_name is an alias for cli_specifier—I think Ridwan is saying to just use zone_name consistently throughout. To actually answer your question: it's NULL when the table has been dropped but its data is still live because of the GC TTL. The table doesn't have a name at that point, so neither does the zone.

---

pkg/server/serverpb/admin.proto, line 567 at r2 (raw file):

  // By zone name.
  map<string, ZoneConfig> zone_configs = 2 [(gogoproto.nullable) = false];

Even though it doesn't really matter because the admin server/client are versioned together, I do think it's good practice to bump the field number whenever its semantics change, if for no other reason than to form the habit.

[vilterp]

TFTRs!

bors r+

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

---

pkg/server/admin.go, line 1540 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

zone_name is an alias for cli_specifier—I think Ridwan is saying to just use zone_name consistently throughout. To actually answer your question: it's NULL when the table has been dropped but its data is still live because of the GC TTL. The table doesn't have a name at that point, so neither does the zone.

Oh right, forgot that the WHERE clause was already there.

Makes sense. Further down the road, I think showing zone configs for deleted data would be good. Will worry about that later though.

---

pkg/server/serverpb/admin.proto, line 567 at r2 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Even though it doesn't really matter because the admin server/client are versioned together, I do think it's good practice to bump the field number whenever its semantics change, if for no other reason than to form the habit.

I buy that. Done.

[benesch]

Reviewed 4 of 4 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

[craig]

Build succeeded

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)