tenantcapabilities: restructure interfaces #100217

ajwerner · 2023-03-30T21:35:46Z

This commit is mostly just a refactoring of what we have. The previous iteration had some unfortunate coupling between the protobufs and the top-level package. Having the protobuf generated code not be a leaf hurts.

Another pain point was the lack of strong typing when interacting with capabilities. Given this is go, somewhere there's a need to do some type assertion when dealing sets of variant instance types.

The biggest change is that the TenantCapabilities concept is no longer wrapped in an interface implemented by the protobuf. Instead, the Capability implementations now live inside the tenantcapabilities package, and can be looked up with FromName and FromID, without any access to concrete capabilities. These capabilies then know how to look up values in a strongly typed way from the concrete capabilities.

Epic: none

Release note: None

cockroach-teamcity · 2023-03-30T21:35:55Z

This change is

arulajmani

This is a great simplification, I love where this ended up -- nice parting gift indeed. Thanks for taking the time to refactor this stuff.

Reviewed 51 of 53 files at r1, 8 of 8 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

-- commits line 6 at r4:

Having the protobuf generated code not be a leaf hurts.

Heavy +1.

pkg/multitenant/tenantcapabilities/value.go line 71 at r3 (raw file):

type spanConfigBoundsValue struct {
	b **tenantcapabilitiespb.SpanConfigBounds

nit: could you add a comment explaining why a pointer to a pointer (as opposed to just a pointer) here?

pkg/multitenant/tenantcapabilities/value.go line 74 at r3 (raw file):

}

func (s *spanConfigBoundsValue) Get() *spanconfigbounds.Bounds {

I like the fact that we're returning spanconfigbounds.Bounds right here, instead of having to convert it in the spanconfig package. I had found myself wrestling with the package structure here initially, so I'm a solid 💯 on this (and the reorganization you've done here, in general).

pkg/multitenant/tenantcapabilities/values.go line 20 at r4 (raw file):

// MustGetValueByID will get the value for the capability corresponding to
// the requested ID. If the ID is not valid, this function will panic.
func MustGetValueByID(t *tenantcapabilitiespb.TenantCapabilities, id ID) Value {

Instead of both this and GetValueByID, consider just having the latter and letting that thing panic if need be. Looks like we're only ever using the version that panics in production anyway, so not having two options here will be one less question future visitors will have to ask when they see this code.

pkg/spanconfig/spanconfigbounds/span_config_bounds.go line 26 at r3 (raw file):

// policy to interact with SpanConfigs.
type Bounds struct {
	b *tenantcapabilitiespb.SpanConfigBounds

Slightly curious why you had this wrapping before but decided to change it.

pkg/spanconfig/spanconfigstore/bounds_reader.go line 11 at r4 (raw file):

// licenses/APL.txt.

package spanconfigstore

Was the package move motivated by not wanting to import spanconfigbounds into KVSubscriber? Or was there more to it?

pkg/multitenant/tenantcapabilities/values_test.go line 20 at r4 (raw file):

)

func TestCapabilityGetSet(t *testing.T) {

This test wasn't great, but do you think it's worth keeping around a TestCapabilitiesGetSet test albeit more useful?

Mostly in the context of span config bounds -- should we add something that uses Set to alter span config bounds and then makes sure the underlying proto is indeed modified using a Get? Your call on whether it's worth having.

ajwerner

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

-- commits line 6 at r4: