spanconfig: introduce the spanconfig.SQLTranslator #71359

arulajmani · 2021-10-09T00:35:19Z

This patch introduces the spanconfig.SQLTranslator which translates
zone configurations to span configurations.

The zone config and span config protos look similar, but they differ
from each other in a couple of ways:

zone configurations correspond to {descriptor, zone} IDs whereas span
configurations correspond to keyspans.
zone configurations have a notion of inheritance which span
configurations do not. Instead, they're fully hydrated and flattened
out.

When the SQLTranslator is given a {zone,descriptor} ID it generates
span configurations for all objects under the given ID in the zone
configuration hierarchy.

The SQLTranslator fills in any missing fields by following up the
inheritance chain to fully hydrate span configs. It also accounts
for subzones (and subzone spans) when constructing (span, span config)
tuples.

Split out from #69661.

Epic: CRDB-10489

Release note: None

cockroach-teamcity · 2021-10-09T00:35:26Z

This change is

arulajmani · 2021-10-09T00:40:19Z

@irfansharif mentioned that this standalone subsystem should really be its own PR on #69661 and I agree. Hopefully this makes reviewing this thing much easier.

This change also accounts for the last round of comments that @irfansharif had on the above PR (hopefully I didn't miss any!). One point of contention on that PR was whether the translator should expect a transaction or create its own -- I spoke with Irfan offline about it and I think we're both aligned on the translator creating its own now.

irfansharif

Nice work! This was plenty easy to review. Left only minor comments.

Reviewed 32 of 32 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, @nvanbenschoten, and @otan)

-- commits, line 5 at r1:
It's more accurate to say that it translates a descriptor and its corresponding zone configs into its constituent spans and span configs. The paragraph below comparing their proto representations and talking about inheritance is less illustrative than just using an actual example -- a snippet from your datadriven test would work very well.

-- commits, line 21 at r1:
[nit] Use "<>" for the tuple syntax?

pkg/ccl/spanconfigccl/datadriven_test.go, line 42 at r1 (raw file):

// It offers the following commands:
//
// "exec-sql": executes the input SQL query.

[nit] Indent these more prominently?

// It offers the following commands:
//
//   "exec-sql"
//   executes the input SQL query
//
//   "translate [database=<string>] [table=<string>] [named-zone=<string>] [id=<int>]"
//   translates the SQL...

pkg/ccl/spanconfigccl/datadriven_test.go, line 49 at r1 (raw file):

// [id=<int>]:
// translates the SQL zone config state to the span config state starting from
// the referenced object (named zone, database, database + table, or id) as the

s/id/descriptor ID?

pkg/ccl/spanconfigccl/datadriven_test.go, line 54 at r1 (raw file):

// "full-translate": performs a full translation of the SQL zone config state
// to the implied span config state. Also see
// spanconfig.SQLTranslator.FullTranslate().

Delete this sentence?

pkg/ccl/spanconfigccl/datadriven_test.go, line 55 at r1 (raw file):

// to the implied span config state. Also see
// spanconfig.SQLTranslator.FullTranslate().
// TODO(arul): Add a secondary tenant configuration for this test as well.

Do you feel adding some datadriven tests to target secondary tenants as part of this PR would be too cumbersome? I think it would be useful.

pkg/ccl/spanconfigccl/datadriven_test.go, line 99 at r1 (raw file):

						objID = descpb.ID(namedZoneID)
					} else {
						t.Fatalf("unkown named zone: %s", zone)

Typo. Also require.Truef(t, found, "unknown ...").

pkg/ccl/spanconfigccl/datadriven_test.go, line 129 at r1 (raw file):

					t.Fatal("insufficient args provided to translate")
				}
				sqlTranslator := tc.Server(0).SpanConfigSQLTranslator().(spanconfig.SQLTranslator)

Pull this sqlTranslator definition outside the switch block.

pkg/ccl/spanconfigccl/datadriven_test.go, line 152 at r1 (raw file):

const (
	id        = "id"
	namedZone = "named-zone"

[nit] Reads more clearly to just inline these strings at the caller.

pkg/ccl/spanconfigccl/datadriven_test.go, line 175 at r1 (raw file):

func diffEntryAgainstRangeDefault(entry roachpb.SpanConfigEntry) string {
	var res strings.Builder
	defaultSpanConfig := zonepb.DefaultZoneConfig().AsSpanConfig()

Use roachpb.TestingDefaultSpanConfig instead.

pkg/ccl/spanconfigccl/testdata/misc, line 23 at r1 (raw file):


# We still should be able to generate the span configuration for it when
# starting our translation from the database.

Mention why. It's also worth testing what happens when the desc is deleted for good, if not too cumbersome (lower gc-ttl + some way to force GC? Is that a thing?)

pkg/server/testserver.go, line 946 at r1 (raw file):

		return ts.sqlServer.spanconfigMgr.SQLTranslator
	}
	return spanconfigsqltranslator.NewDisabled()

Better than this hollow disabled type, just panic("something informative; see some relevant testing field"). It's going to error out if you don't set the relevant testing field anyway -- feels equally ergonomic to test authors (just us for some time).

pkg/spanconfig/spanconfig.go, line 38 at r1 (raw file):

}

// SQLTranslator translates SQL descriptors and their corresponding zone

"their corresponding zone configs to constituent spans and corresponding span configs". The second sentence doesn't feel appropriate here and could be removed. Including a tiny example here (similar to the commit message thread above) would help.

pkg/spanconfig/spanconfig.go, line 43 at r1 (raw file):

	KVAccessor

	// TODO(irfansharif): We'll also want access to a "SQLWatcher", something

This TODO still applies partially.

pkg/spanconfig/spanconfig.go, line 49 at r1 (raw file):

	// FullTranslate translates the entire SQL zone configuration state to the
	// implied span configuration state. The timestamp at which such a translation
	// is valid is al so returned.

"also".

pkg/spanconfig/spanconfigsqltranslator/disabled.go, line 25 at r1 (raw file):

// DisabledSQLTranslator is an empty implementation of spanconfig.SQLTranslator
// for testing purposes.
type DisabledSQLTranslator struct{}

See above, should go away.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 74 at r1 (raw file):

		) error {
			for _, id := range ids {
				idEntries, err := s.translateID(ctx, id, txn, descsCol)

s/idEntries/translatedEntries? Ditto below.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 80 at r1 (raw file):

				entries = append(entries, idEntries...)
			}
			// TODO(arul): Looks like the use of this method means the transactions

Seems fine, if it gets pushed it'll fail with a retryable error and we're in a retryable closure anyway.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 89 at r1 (raw file):

}

// translateID generates the implied span configuration state for a single ID.

s/single/given descriptor

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 93 at r1 (raw file):

	ctx context.Context, id descpb.ID, txn *kv.Txn, descsCol *descs.Collection,
) (entries []roachpb.SpanConfigEntry, err error) {
	var expandedIDs descpb.IDs

"expanded" seems wrong but I can't think of something better; feels more "descendant" or "children" if anything.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 95 at r1 (raw file):

	var expandedIDs descpb.IDs
	if zonepb.IsNamedZoneID(id) {
		entries, expandedIDs, err = s.translateNamedZoneConfig(ctx, id, txn, descsCol)

s/translateNamedZoneConfig/translateNamedZoneID
s/translateDescriptorIDZoneConfig/translateDescriptorID

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 131 at r1 (raw file):

	if err != nil {
		if errors.Is(err, catalog.ErrDescriptorNotFound) {
			// The descriptor has been deleted. If this is the case then no span

s/If this is the case.../Nothing to do here.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 190 at r1 (raw file):

	case zonepb.DefaultZoneName:
	default:
		// No IDs lie below in the zone configuration hierarchy for named zones

Convert this fall-through single case switch block to a simple if statement?

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 195 at r1 (raw file):

	}

	// A change to RANGE DEFAULT may imply a change to every SQL object for the

s/may imply/translates to every SQL object...

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 210 at r1 (raw file):

	// As all named zones also inherit from RANGE DEFAULT, a change to it may also
	// imply a change to every other named zone. This is only a thing for the

Ditto, drop the "imply" language. At the level of this component, we're just blindly translating. Whether the changes are implied or not will be handled at the reconciler.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 216 at r1 (raw file):

		for _, namedZone := range zonepb.NamedZonesList {
			// Add an entry for all named zones bar RANGE DEFAULT.
			if namedZone != zonepb.DefaultZoneName {

[nit] if namedZone == zonepb.DefaultZoneName { continue }

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 238 at r1 (raw file):

			// `system.span_configurations` as this span is at the tenant boundary,
			// and as such, a hard split point.
			spans = append(spans, roachpb.Span{

I remember us adding this snippet in our prototype branch to ensure splits on tenant boundaries, but it's a bit odd to see it here out of context. I'd be ok leaving this out for now.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 270 at r1 (raw file):

	}

	zone, err := sql.GetHydratedZoneConfigForNamedZone(ctx, txn, s.codec, name)

s/zone/zoneConfig. Ditto for the method below.

pkg/sql/zone_config.go, line 273 at r1 (raw file):

		return kv.Value, nil
	}
	id := zonepb.NamedZones[zoneName]

error.AssertionFailed if named zone not found?

pkg/sql/zone_config.go, line 295 at r1 (raw file):

		return kv.Value, nil
	}
	// TODO(zcfgs-pod): Use the descCollection to do descriptor lookups instead of this

Are you looking to do this now? If not, use your name for the TODO instead.

pkg/sql/catalog/descs/collection.go, line 284 at r1 (raw file):

// GetAllTableDescriptorsInDatabase returns all the table descriptors visible to
// the transaction inside the database referenced by the given database ID. It

s/inside .../under the database with the given ID.

pkg/sql/catalog/descs/collection.go, line 285 at r1 (raw file):

// GetAllTableDescriptorsInDatabase returns all the table descriptors visible to
// the transaction inside the database referenced by the given database ID. It
// first checks the collections cached descriptors before defaulting to a key-value scan.

", if necessary."

irfansharif

Woops, pressed the wrong button.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, @nvanbenschoten, and @otan)

ajwerner

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @nvanbenschoten, and @otan)

pkg/spanconfig/spanconfig.go, line 46 at r1 (raw file):

	// {descriptor, named zone} IDs. No entry is returned for an ID if it doesn't
	// exist or has been dropped.
	Translate(ctx context.Context, ids descpb.IDs) ([]roachpb.SpanConfigEntry, error)

How would you feel about the idea of exposing the whole RootNamespaceID as the key to reconcile everything and making this one method? It feels to me like returning a timestamp always is not a bad idea.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 108 at r1 (raw file):

	for _, ID := range expandedIDs {
		idEntries, err := s.translateID(ctx, ID, txn, descsCol)

do you like this recursion? I feel like it's hard to tell whether we might create multiple entries for the same span. Consider when you've got a table ID and its parent in the set. How do you feel about refactoring this code such that expansions is separate from entry generation? That way, you could avoid any recursion in entry generation.

nvanbenschoten

Reviewed 32 of 32 files at r1, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @ajwerner, @arulajmani, @irfansharif, and @otan)

pkg/ccl/spanconfigccl/testdata/misc, line 3 at r1 (raw file):

# Miscellaneous edge case tests for the SQLTranslator.

# Test dropped tables/databases work correctly.

Do we want to test offline descriptors?

pkg/config/zonepb/zone.go, line 45 at r1 (raw file):

)

// NamedZonesList is a list of all named zones that that reference ranges

"that that"

pkg/spanconfig/spanconfig.go, line 46 at r1 (raw file):

Previously, ajwerner wrote…

How would you feel about the idea of exposing the whole RootNamespaceID as the key to reconcile everything and making this one method? It feels to me like returning a timestamp always is not a bad idea.

We discussed this together last Tuesday and there were differing opinions. Arguments in favor of consolidating these into a single method were that it avoids duplication and reduces the burden on implementors of this method. Arguments for the separation of these methods were that it allows us to represent the concept of a "full translation" directly in the code, it may allow for a more efficient implementation of full translation using a single scan of descriptors and zones instead of recursive lookups, and it avoids callers needing to know that a sentinel RootNamespaceID value has a certain meaning.

Perhaps a compromise that retains all of these benefits is to define FullTranslate as a "default method" and in terms of Translate. In Go, a default method just looks like an exported function that lives alongside the interface definition:

// SQLTranslator ...
type SQLTranslator interface {
    // Translate ...
    Translate(ctx context.Context, ids descpb.IDs) ([]roachpb.SpanConfigEntry, hlc.Timestamp, error)
}

// FullTranslate ...
func FullTranslate(ctx context.Context, st SQLTranslator) ([]roachpb.SpanConfigEntry, hlc.Timestamp, error) {
    // ... some comment about why this works ...
    return st.Translate(ctx, descpb.IDs{keys.RootNamespaceID})
}

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 66 at r1 (raw file):

// translate generates the implied span configuration state for the given IDs.
func (s *SQLTranslator) translate(

We had talked with Irfan about whether a Txn should be provided to this function or not. Where did we land on that?

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 69 at r1 (raw file):

	ctx context.Context, ids descpb.IDs,
) (entries []roachpb.SpanConfigEntry, timestamp hlc.Timestamp, err error) {
	err = sql.DescsTxn(

nit: this indentation is pretty tough to read, IMO. Unless you have a good reason for it, consider just formatting this the way every other use of sql.DescsTxn does:

err := sql.DescsTxn(ctx, &execCfg, func(
	ctx context.Context, txn *kv.Txn, descriptors *descs.Collection,
) error {
    ...
}

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 80 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Seems fine, if it gets pushed it'll fail with a retryable error and we're in a retryable closure anyway.

The "better" (but not particularly great) way to do this is to save the txn in some local variable and then call CommitTimestamp after the txn has been committed and returned a nil error.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 93 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

"expanded" seems wrong but I can't think of something better; feels more "descendant" or "children" if anything.

+1

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 125 at r1 (raw file):

) (entries []roachpb.SpanConfigEntry, expandedIDs descpb.IDs, err error) {
	desc, err := descsCol.GetImmutableDescriptorByID(ctx, txn, id, tree.CommonLookupFlags{
		AvoidCached:    true,

Does the configuration of each of these flags deserve a comment?

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 143 at r1 (raw file):

	// There's nothing to do for {Type,Schema} descriptors.
	if desc.DescriptorType() == catalog.Type || desc.DescriptorType() == catalog.Schema {

Consider using a switch statement here so that we have to exhaustively check each descriptor type and so this blows up if a new descriptor type is added.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 368 at r1 (raw file):

	// No descriptors lie below {Table,Type,Schema} descriptors in the zone config
	// hierarchy.
	if desc.DescriptorType() == catalog.Table || desc.DescriptorType() == catalog.Type || desc.DescriptorType() == catalog.Schema {

Same point about exhaustively handling descriptor types.

pkg/sql/zone_config.go, line 273 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

error.AssertionFailed if named zone not found?

+1

pkg/sql/catalog/descs/collection.go, line 285 at r1 (raw file):

// GetAllTableDescriptorsInDatabase returns all the table descriptors visible to
// the transaction inside the database referenced by the given database ID. It
// first checks the collections cached descriptors before defaulting to a key-value scan.

"collection's"

pkg/sql/catalog/descs/collection.go, line 291 at r1 (raw file):

	// Ensure the given ID does indeed belong to a database.
	found, _, err := tc.getDatabaseByID(ctx, txn, dbID, tree.DatabaseLookupFlags{
		AvoidCached: false,

Should this IncludeOffline and IncludeDropped as well?

arulajmani

Addressed all comments. @ajwerner do you want to have another quick look before I bors this thing?

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @arulajmani, @irfansharif, @nvanbenschoten, and @otan)

-- commits, line 5 at r1:

Previously, irfansharif (irfan sharif) wrote…

It's more accurate to say that it translates a descriptor and its corresponding zone configs into its constituent spans and span configs. The paragraph below comparing their proto representations and talking about inheritance is less illustrative than just using an actual example -- a snippet from your datadriven test would work very well.

Done.

-- commits, line 21 at r1:

Previously, irfansharif (irfan sharif) wrote…

[nit] Use "<>" for the tuple syntax?

Done.

pkg/ccl/spanconfigccl/datadriven_test.go, line 42 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

[nit] Indent these more prominently?

// It offers the following commands:
//
//   "exec-sql"
//   executes the input SQL query
//
//   "translate [database=<string>] [table=<string>] [named-zone=<string>] [id=<int>]"
//   translates the SQL...

Done.

pkg/ccl/spanconfigccl/datadriven_test.go, line 49 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/id/descriptor ID?

Done.

pkg/ccl/spanconfigccl/datadriven_test.go, line 54 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Delete this sentence?

Done.

pkg/ccl/spanconfigccl/datadriven_test.go, line 55 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Do you feel adding some datadriven tests to target secondary tenants as part of this PR would be too cumbersome? I think it would be useful.

I'd rather do it as a quickly followed up PR to this. Filed #71733 to not lose track.

pkg/ccl/spanconfigccl/datadriven_test.go, line 99 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Typo. Also require.Truef(t, found, "unknown ...").

Done.

pkg/ccl/spanconfigccl/datadriven_test.go, line 129 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Pull this sqlTranslator definition outside the switch block.

Done.

pkg/ccl/spanconfigccl/datadriven_test.go, line 175 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Use roachpb.TestingDefaultSpanConfig instead.

Done.

pkg/ccl/spanconfigccl/testdata/misc, line 3 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we want to test offline descriptors?

Good call, done.

pkg/ccl/spanconfigccl/testdata/misc, line 23 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Mention why. It's also worth testing what happens when the desc is deleted for good, if not too cumbersome (lower gc-ttl + some way to force GC? Is that a thing?)

I've to introduce sleeps into this thing so that the GC job gets a chance to run. Setting the ttl to 1 second and sleeping for 5 -- hopefully this isn't flaky.

pkg/server/testserver.go, line 946 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Better than this hollow disabled type, just panic("something informative; see some relevant testing field"). It's going to error out if you don't set the relevant testing field anyway -- feels equally ergonomic to test authors (just us for some time).

Done.

pkg/spanconfig/spanconfig.go, line 38 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

"their corresponding zone configs to constituent spans and corresponding span configs". The second sentence doesn't feel appropriate here and could be removed. Including a tiny example here (similar to the commit message thread above) would help.

Done.

pkg/spanconfig/spanconfig.go, line 43 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

This TODO still applies partially.

Updated it to describe the flow in the new world. Hopefully it's a short lived paragraph!

pkg/spanconfig/spanconfig.go, line 46 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We discussed this together last Tuesday and there were differing opinions. Arguments in favor of consolidating these into a single method were that it avoids duplication and reduces the burden on implementors of this method. Arguments for the separation of these methods were that it allows us to represent the concept of a "full translation" directly in the code, it may allow for a more efficient implementation of full translation using a single scan of descriptors and zones instead of recursive lookups, and it avoids callers needing to know that a sentinel RootNamespaceID value has a certain meaning.

Perhaps a compromise that retains all of these benefits is to define FullTranslate as a "default method" and in terms of Translate. In Go, a default method just looks like an exported function that lives alongside the interface definition:
// SQLTranslator ...
type SQLTranslator interface {
    // Translate ...
    Translate(ctx context.Context, ids descpb.IDs) ([]roachpb.SpanConfigEntry, hlc.Timestamp, error)
}

// FullTranslate ...
func FullTranslate(ctx context.Context, st SQLTranslator) ([]roachpb.SpanConfigEntry, hlc.Timestamp, error) {
    // ... some comment about why this works ...
    return st.Translate(ctx, descpb.IDs{keys.RootNamespaceID})
}

@irfansharif was pushing for this as well, but I wanted to keep the concept of FullTranslate <-> Translate(root namespace id) within this Translator's abstraction boundary. I was imagining we'd only need a timestamp after a full translation is done to use as the start timestamp for the SQLWatcher to establish its rangefeeds. For all other checkpointing in the span config job we would only ever need the SQLWatcher's frontier timestamp (and not the timestamp of the transaction used to react to the frontier getting bumped). Did you have a particular case in mind where returning the timestamp could be useful?

Separately, I'm happy to make this a single method though I do have a mild preference of keeping these as is if left to my own devices.

Edit: I wrote the thing above before Nathan's comment. Switched it to making FullTranslate a public method on the translator instead of an interface method.

pkg/spanconfig/spanconfig.go, line 49 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

"also".

Done.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 66 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We had talked with Irfan about whether a Txn should be provided to this function or not. Where did we land on that?

We spoke about it a bit more offline after our discussion and Irfan came around to having the Translator construct it's own transaction (instead of accepting it as an argument.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 69 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: this indentation is pretty tough to read, IMO. Unless you have a good reason for it, consider just formatting this the way every other use of sql.DescsTxn does:
err := sql.DescsTxn(ctx, &execCfg, func(
	ctx context.Context, txn *kv.Txn, descriptors *descs.Collection,
) error {
    ...
}

No good reason, in fact, I misread @ajwerner's earlier suggestion about exactly this from the other branch! Thanks for catching.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 74 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/idEntries/translatedEntries? Ditto below.

Done.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 80 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Seems fine, if it gets pushed it'll fail with a retryable error and we're in a retryable closure anyway.

Done.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 89 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/single/given descriptor

Done.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 93 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

+1

I'm referring to these as "descendant leaf" objects in the zone configuration hierarchy now. I'm not fully satisfied with it, open to other alternatives.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 95 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/translateNamedZoneConfig/translateNamedZoneID
s/translateDescriptorIDZoneConfig/translateDescriptorID

Done.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 108 at r1 (raw file):

Previously, ajwerner wrote…

do you like this recursion? I feel like it's hard to tell whether we might create multiple entries for the same span. Consider when you've got a table ID and its parent in the set. How do you feel about refactoring this code such that expansions is separate from entry generation? That way, you could avoid any recursion in entry generation.

I like this suggestion quite a lot, done. Makes the flow much easier to understand. Do you think it's worth abstracting the expansion logic into its own struct? It feels like a good boundary, but Im struggling to find a good name for this expansion process to do so. Thoughts?

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 131 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/If this is the case.../Nothing to do here.

Done.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 190 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Convert this fall-through single case switch block to a simple if statement?

Done.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 195 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/may imply/translates to every SQL object...

Done.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 210 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Ditto, drop the "imply" language. At the level of this component, we're just blindly translating. Whether the changes are implied or not will be handled at the reconciler.

Good point, some of this language was from when we hadn't carved out the translator concept. Got rid.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 216 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

[nit] if namedZone == zonepb.DefaultZoneName { continue }

Done.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 238 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

I remember us adding this snippet in our prototype branch to ensure splits on tenant boundaries, but it's a bit odd to see it here out of context. I'd be ok leaving this out for now.

Done.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 270 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/zone/zoneConfig. Ditto for the method below.

Done.

pkg/sql/zone_config.go, line 273 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

error.AssertionFailed if named zone not found?

Done.

pkg/sql/zone_config.go, line 295 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Are you looking to do this now? If not, use your name for the TODO instead.

This would require a fair bit of refactoring considering how coupled this code is with in memory lookups in the system config span, so I don't see us doing it before we rip that out.

pkg/sql/catalog/descs/collection.go, line 284 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/inside .../under the database with the given ID.

Done.

pkg/sql/catalog/descs/collection.go, line 285 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

", if necessary."

Done.

pkg/sql/catalog/descs/collection.go, line 291 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should this IncludeOffline and IncludeDropped as well?

Not for our use-case, but I'll defer to @ajwerner considering I'm adding this to the desc collection.

pkg/spanconfig/spanconfigsqltranslator/disabled.go, line 25 at r1 (raw file):

Previously, irfansharif (irfan sharif) wrote…

See above, should go away.

Done.

nvanbenschoten

Reviewed 14 of 17 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @arulajmani, @irfansharif, @nvanbenschoten, and @otan)

pkg/spanconfig/spanconfig.go, line 46 at r1 (raw file):

Switched it to making FullTranslate a public method on the translator instead of an interface method.

I'm a bit confused. So it's an exported method on the concrete type but no longer part of the interface or pkg/spanconfig API? Are you sure that's what you want? Will users have access to the concrete type?

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 80 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Done.

I think you only want to do this if err == nil.

arulajmani

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @arulajmani, @irfansharif, @nvanbenschoten, and @otan)

pkg/spanconfig/spanconfig.go, line 46 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Switched it to making FullTranslate a public method on the translator instead of an interface method.

I'm a bit confused. So it's an exported method on the concrete type but no longer part of the interface or pkg/spanconfig API? Are you sure that's what you want? Will users have access to the concrete type?

Oops my bad, I completely misread your comment above 😅

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 80 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I think you only want to do this if err == nil.

Done.

ajwerner

Reviewed 1 of 17 files at r2, 1 of 4 files at r3, 2 of 3 files at r4.
Reviewable status: complete! 2 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @arulajmani, @irfansharif, @nvanbenschoten, and @otan)

pkg/ccl/spanconfigccl/datadriven_test.go, line 278 at r4 (raw file):

Quoted 13 lines of code…

	if len(diffs) != 0 {
		for i, diff := range diffs {
			if i == 0 {
				res.WriteString(fmt.Sprintf("%-30s %s", entry.Span.String(), diff))
			} else {
				res.WriteByte(' ')
				res.WriteString(diff)
			}
		}
	} else {
		res.WriteString(fmt.Sprintf("%-30s %s", entry.Span.String(), "DEFAULT"))
	}
	return res.String()

nit: I think this all can be just

if len(diffs) == 0 {
    diffs = []string{"DEFAULT"}
}
return fmt.Sprintf("%-30s %s", entry.Span.String(), strings.Join(diffs, " "))

pkg/spanconfig/spanconfig.go, line 84 at r4 (raw file):

Quoted 5 lines of code…

// ReconciliationDependencies captures what's needed by the span config
// reconciliation job to perform its task. The job is responsible for
// reconciling a tenant's zone configurations with the clusters span
// configurations.
type ReconciliationDependencies interface {

this is good for expressing intention now. When the job comes to exist, it may make sense to just define this closer to it. This definition isn't really needed for users of this subsystem.

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 11 at r4 (raw file):

// licenses/APL.txt.

package spanconfigsqltranslator

nit: want to put a doc comment here?

pkg/spanconfig/spanconfigsqltranslator/sql_translator.go, line 60 at r4 (raw file):

		// We're in a retryable closure, so clear any entries from previous
		// attempts.
		entries = make([]roachpb.SpanConfigEntry, 0)

nit: could be entries = entries[:0]

pkg/sql/zone_config.go, line 295 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

This would require a fair bit of refactoring considering how coupled this code is with in memory lookups in the system config span, so I don't see us doing it before we rip that out.

Yeah, this is a bigger project that the schema team is interested in the medium term. Maybe January, maybe next release.

This patch introduces the spanconfig.SQLTranslator which translates a descriptor and its corresponding zone configurations into its constituent span configurations. The zone config and span config protos look similar, but they differ from each other in a couple of ways: - zone configurations correspond to {descriptor, zone} IDs whereas span configurations correspond to keyspans. - zone configurations have a notion of inheritance which span configurations do not. Instead, they're fully hydrated and flattened out. When the SQLTranslator is given a {zone,descriptor} ID it first descends the zone configuration hierarchy starting from the ID to accumulate IDs of all leaf objects. Leaf objects are tables and named zones (other than RANGE DEFAULT) which have actual span configurations associated with them. For every one of these accumulated IDs, it then generates <span, span config> tuples by following up the inheritance chain to fully hydrate the span config. The SQLTranslator also accounts for and negotiates subzones in this process. Concretely, for the following zone configuration hierarchy: ``` CREATE DATABASE db; CREATE TABLE db.t1(); ALTER DATABASE db CONFIGURE ZONE USING num_replicas=7; ALTER TABLE db.t1 CONFIGURE ZONE USING num_voters=5; ``` The SQLTranslator produces the following translation (represented as a diff against RANGE DEFAULT for brevity): ``` /Table/5{3-4} num_replicas=7 num_voters=5 ``` Split out from cockroachdb#69661. Release note: None

arulajmani

TFTRs! Will bors on green.

Reviewable status: complete! 1 of 0 LGTMs obtained (and 2 stale) (waiting on @ajwerner, @arulajmani, @irfansharif, @nvanbenschoten, and @otan)

pkg/spanconfig/spanconfig.go, line 84 at r4 (raw file):

Previously, ajwerner wrote…

// ReconciliationDependencies captures what's needed by the span config
// reconciliation job to perform its task. The job is responsible for
// reconciling a tenant's zone configurations with the clusters span
// configurations.
type ReconciliationDependencies interface {
this is good for expressing intention now. When the job comes to exist, it may make sense to just define this closer to it. This definition isn't really needed for users of this subsystem.

Yeah, I think once we have the Reconciler it'll encapsulate all dependencies and we won't need this things for then.

nvanbenschoten

Reviewed 1 of 4 files at r3, 2 of 3 files at r4, 3 of 3 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @arulajmani, @irfansharif, @nvanbenschoten, and @otan)

arulajmani · 2021-10-20T15:23:31Z

TFTRs! (on this, and the PR this was split out from)

bors r=irfansharif,nvanbenschoten,ajwerner

craig · 2021-10-20T17:00:34Z

Build succeeded:

GitHub CI (Cockroach)

arulajmani requested review from ajwerner, nvanbenschoten and irfansharif October 9, 2021 00:35

arulajmani requested review from a team as code owners October 9, 2021 00:35

arulajmani requested a review from a team October 9, 2021 00:35

arulajmani requested a review from a team as a code owner October 9, 2021 00:35

arulajmani requested review from otan and removed request for a team October 9, 2021 00:35

irfansharif mentioned this pull request Oct 13, 2021

reports: generate replication reports for secondary tenants #70557

Closed

irfansharif suggested changes Oct 14, 2021

View reviewed changes

irfansharif approved these changes Oct 14, 2021

View reviewed changes

irfansharif mentioned this pull request Oct 14, 2021

*: rework zone configurations to support tenants #67679

Closed

21 tasks

ajwerner reviewed Oct 18, 2021

View reviewed changes

nvanbenschoten approved these changes Oct 19, 2021

View reviewed changes

arulajmani mentioned this pull request Oct 19, 2021

spanconfig: add secondary tenant config for SQLTranslator datadriven tests #71733

Closed

arulajmani force-pushed the spanconfigs-sql-translator branch from 6cc313a to b707711 Compare October 20, 2021 00:24

arulajmani commented Oct 20, 2021

View reviewed changes

nvanbenschoten reviewed Oct 20, 2021

View reviewed changes

arulajmani force-pushed the spanconfigs-sql-translator branch 2 times, most recently from 1e8a957 to c09264b Compare October 20, 2021 03:38

arulajmani commented Oct 20, 2021

View reviewed changes

arulajmani force-pushed the spanconfigs-sql-translator branch from c09264b to ca0faae Compare October 20, 2021 04:09

ajwerner approved these changes Oct 20, 2021

View reviewed changes

arulajmani force-pushed the spanconfigs-sql-translator branch from ca0faae to 6429466 Compare October 20, 2021 14:23

arulajmani force-pushed the spanconfigs-sql-translator branch from 6429466 to d3856eb Compare October 20, 2021 14:24

arulajmani commented Oct 20, 2021

View reviewed changes

nvanbenschoten reviewed Oct 20, 2021

View reviewed changes

craig bot merged commit 85b0b8a into cockroachdb:master Oct 20, 2021

irfansharif mentioned this pull request Nov 13, 2021

spanconfig: introduce the spanconfig.SQLWatcher #69661

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spanconfig: introduce the spanconfig.SQLTranslator #71359

spanconfig: introduce the spanconfig.SQLTranslator #71359

arulajmani commented Oct 9, 2021 •

edited by ajwerner

Loading

cockroach-teamcity commented Oct 9, 2021

arulajmani commented Oct 9, 2021

irfansharif left a comment

irfansharif left a comment

ajwerner left a comment

nvanbenschoten left a comment

arulajmani left a comment

nvanbenschoten left a comment

arulajmani left a comment

ajwerner left a comment

arulajmani left a comment

nvanbenschoten left a comment

arulajmani commented Oct 20, 2021

craig bot commented Oct 20, 2021

spanconfig: introduce the spanconfig.SQLTranslator #71359

spanconfig: introduce the spanconfig.SQLTranslator #71359

Conversation

arulajmani commented Oct 9, 2021 • edited by ajwerner Loading

cockroach-teamcity commented Oct 9, 2021

arulajmani commented Oct 9, 2021

irfansharif left a comment

Choose a reason for hiding this comment

irfansharif left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

arulajmani commented Oct 20, 2021

craig bot commented Oct 20, 2021

arulajmani commented Oct 9, 2021 •

edited by ajwerner

Loading