config/storage: cache zone config values to avoid repetitive deserialization

[spencerkimball]

This change allows a SystemConfig to cache object ID -> ZoneConfig
lookups to avoid repetitively deserializing table descriptors and zone
configs for every replica in the system. Replicas themselves now also
cache zone config entries to avoid maintaining per-replica min/max bytes
and for making future use of additional zone config parameters simpler
(e.g. zone config GC policies, number of replicas, etc.).

Fixes #21702

[cockroach-teamcity]

This change is 

[nvanbenschoten]

Mechanically this However, I'm not familiar enough with zone configs and especially subzones and placeholder zones to be able to comment about them.

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

---

pkg/storage/replica_proposal.go, line 537 at r2 (raw file):

	if needsSplitBySize {
		log.Infof(context.TODO(), "zone for %s: %+v", r, r.GetZoneConfig())

Did you mean to remove this?

[spencerkimball]

Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/storage/replica_proposal.go, line 537 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Did you mean to remove this?

Done.

[spencerkimball]

Added @benesch to review.

[benesch]

Whew, that looked like it was painful. Thanks for finally fixing this! And sorry I didn't get to this yesterday; I got stuck tracking down fallout from the go1.11 bump.

The only thing I'd like to get resolved before merging is my confusion about the cache return parameter from sql.ZoneConfigHook. Feel free to ignore everything else.

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

---

pkg/config/system.go, line 201 at r1 (raw file):

// GetZoneConfigForKey looks up the zone config for the object (table
// or database, specified by key.id) and optional partition (specified
// by key.suffix). It is the caller's responsibility to ensure that

Is this true? Seems like key is still just a roachpb.RKey. I think maybe this comment should have gone on getZoneConfigForKey?

---

pkg/config/system.go, line 244 at r1 (raw file):

		} else if zone != nil {
			entry = zoneEntry{zone: zone, placeholder: placeholder}
			if cache {

What's this cache param about? It seems to always be equivalent to err != nil.

---

pkg/sql/show_zone_config.go, line 109 at r1 (raw file):

			zone = &subzone.Config
		}
	}

I think I'd have preferred to keep this complexity in GetZoneConfigInTxn, since this chunk of logic is duplicated in setZoneConfig and showZoneConfig, and verifyZoneConfigs. Feel free to leave to a follow up, since I think Ridwan's going to be doing cleanup in the area anyway.

---

pkg/storage/replica.go, line 6735 at r1 (raw file):

	kvs := br.Responses[0].GetInner().(*roachpb.ScanResponse).Rows
	sysCfg := config.NewSystemConfig()
	sysCfg.Values = kvs

Food for thought: perhaps NewSystemConfig should take the values as an argument.

---

pkg/storage/store_rebalancer.go, line 391 at r1 (raw file):

		desc, zone := replWithStats.repl.DescAndZone()
		if zone == nil {

Could this really be nil? It seems like repl.mu.zone is always set to a non-nil zone config.

Superseded by Nikhil's review

[spencerkimball]

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

---

pkg/config/system.go, line 201 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Is this true? Seems like key is still just a roachpb.RKey. I think maybe this comment should have gone on getZoneConfigForKey?

You're right. Fixed.

---

pkg/config/system.go, line 244 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

What's this cache param about? It seems to always be equivalent to err != nil.

The cache param is returned by the zone config hook. It specifies whether the hook allows its results to be cached by the SystemConfig. This is always true in production, but not true in some test cases where there's a testing hook set.

---

pkg/sql/show_zone_config.go, line 109 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

I think I'd have preferred to keep this complexity in GetZoneConfigInTxn, since this chunk of logic is duplicated in setZoneConfig and showZoneConfig, and verifyZoneConfigs. Feel free to leave to a follow up, since I think Ridwan's going to be doing cleanup in the area anyway.

No, that's a solid idea. I wasn't sure about the various ways that GetZoneConfigInTxn was used when I started this. I'll move this code back into that method and return the subzone from there instead.

---

pkg/storage/replica.go, line 6735 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Food for thought: perhaps NewSystemConfig should take the values as an argument.

In this case we don't want to use NewSystemConfig because we want the underlying protobuf, not the caching parent struct

---

pkg/storage/store_rebalancer.go, line 391 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Could this really be nil? It seems like repl.mu.zone is always set to a non-nil zone config.

It is in some test conditions and it's a pain to work around it from the test itself. (e.g. storage.TestChooseLeaseToTransfer)

[benesch]

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

---

pkg/config/system.go, line 244 at r1 (raw file):

Previously, spencerkimball (Spencer Kimball) wrote…

The cache param is returned by the zone config hook. It specifies whether the hook allows its results to be cached by the SystemConfig. This is always true in production, but not true in some test cases where there's a testing hook set.

Ah! Thanks, missed that.

Now that zone configs are accessible from SQL we can probably get rid of that testing hook with some elbow grease. (Definitely not suggested that you do that.)

---

pkg/storage/replica.go, line 6735 at r1 (raw file):

Previously, spencerkimball (Spencer Kimball) wrote…

In this case we don't want to use NewSystemConfig because we want the underlying protobuf, not the caching parent struct

Ack. That comment was on an earlier revision of your PR. Design as-is LGTM.

---

pkg/storage/store_rebalancer.go, line 391 at r1 (raw file):

Previously, spencerkimball (Spencer Kimball) wrote…

It is in some test conditions and it's a pain to work around it from the test itself. (e.g. storage.TestChooseLeaseToTransfer)

Seems like you did, though? 🙌

[spencerkimball]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)