spanconfigsqltranslator: emit all SystemTarget span configs when required #76606
Conversation
|
This change is |
|
First two commits from #76414, will rebase once it merges. |
adityamaru
force-pushed
the
teach-translator
branch
from
cd07089
to
a0b01e0
Compare
adityamaru
force-pushed
the
teach-translator
branch
from
a0b01e0
to
d578aa5
Compare
There was a problem hiding this comment.
I mostly have nitty comments! Good stuff 💯
Reviewed 8 of 9 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @irfansharif)
pkg/roachpb/span_config.go, line 113 at r1 (raw file):
// TestingSystemTargetDefaultSpanConfig exports the default span config that // applies to spanconfig.SystemTargets for testing purposes. func TestingSystemTargetDefaultSpanConfig() SpanConfig {
Should we call this TestingDefaultSystemSpanConfiguration?
pkg/spanconfig/spanconfig.go, line 116 at r1 (raw file):
// configuration. Translate also accounts for and negotiates subzone spans. Translate(ctx context.Context, ids descpb.IDs, shouldGenerateSystemTargetRecords bool) ([]Record, hlc.Timestamp, error)
Similar to my comment above, should we call this generateSystemSpanConfigurations?
pkg/spanconfig/spanconfigreconciler/reconciler.go, line 382 at r1 (raw file):
allIDs = append(allIDs, update.GetDescriptorUpdate().ID) } else if update.IsProtectedTimestampUpdate() { shouldGenerateSystemTargetRecords = true
Should we revert this until we teach + test system span configurations to the reconciler? Wiring this up right now, without the KV changes, might cause issues.
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 157 at r1 (raw file):
// generateSystemTargetRecords is responsible for generating all the SpanConfigs // that apply to spanconfig.SystemTargets. func (s *SQLTranslator) generateSystemTargetRecords(
s/generateSystemTargetRecords/generateSystemSpanConfigRecords/?
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 187 at r1 (raw file):
// Aggregate tenant target protections. tenantProtections := ptsStateReader.getProtectionPoliciesForTenants()
Centralizing the discussion we've had in a couple of places, do you prefer this approach over returning ptpb.Target's from the ptsStateReader and writing conversions from a ptpb.Target to a spanconfig.Target (and vice versa)?
pkg/spanconfig/spanconfigtestutils/utils.go, line 277 at r1 (raw file):
// diffs the given config against the default config that applies to // spanconfig.SystemTargets, and returns a string for the mismatched fields. func PrintSystemTargetSpanConfigDiffedAgainstDefault(conf roachpb.SpanConfig) string {
s/PrintSystemTargetSpanConfigDiffedAgainstDefault/PrintSystemSpanConfigDiffedAgainstDefault/g
pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go, line 174 at r1 (raw file):
output.WriteString(fmt.Sprintf("%-42s %s\n", record.Target.GetSpan(), spanconfigtestutils.PrintSpanConfigDiffedAgainstDefaults(record.Config))) } else {
nit: might be worth using a switch statement which explicitly checks for IsSpanTarget and IsSystemTarget and panics in the default case.
|
Going to wait for #76721 before I rebase this PR. |
There was a problem hiding this comment.
Reviewable status:
pkg/roachpb/span_config.go, line 113 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Should we call this
TestingDefaultSystemSpanConfiguration?
Done.
pkg/spanconfig/spanconfig.go, line 116 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Similar to my comment above, should we call this
generateSystemSpanConfigurations?
Done.
pkg/spanconfig/spanconfigreconciler/reconciler.go, line 382 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Should we revert this until we teach + test system span configurations to the reconciler? Wiring this up right now, without the KV changes, might cause issues.
Sounds good, added a todo.
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 157 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
s/generateSystemTargetRecords/generateSystemSpanConfigRecords/?
Done.
pkg/spanconfig/spanconfigsqltranslator/sqltranslator.go, line 187 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Centralizing the discussion we've had in a couple of places, do you prefer this approach over returning ptpb.Target's from the ptsStateReader and writing conversions from a ptpb.Target to a spanconfig.Target (and vice versa)?
After the discussions/refactor of the methods in your other PRs, I'm happy with where this is right now. If we think there's some cleanup to be done in the future we can revisit.
pkg/spanconfig/spanconfigtestutils/utils.go, line 277 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
s/PrintSystemTargetSpanConfigDiffedAgainstDefault/PrintSystemSpanConfigDiffedAgainstDefault/g
Done.
pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go, line 174 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
nit: might be worth using a switch statement which explicitly checks for
IsSpanTargetandIsSystemTargetand panics in the default case.
Done.
adityamaru
force-pushed
the
teach-translator
branch
from
d578aa5
to
cbb2f78
Compare
|
@arulajmani rebased, and addressed all your comments. Should be RFAL |
There was a problem hiding this comment.
Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status:
pkg/spanconfig/spanconfigtestutils/utils.go, line 280 at r2 (raw file):
// PrintSystemSpanConfigDiffedAgainstDefault is a helper function that // diffs the given config against the default config that applies to
nit: "the default system span config"
pkg/spanconfig/spanconfigtestutils/utils.go, line 284 at r2 (raw file):
func PrintSystemSpanConfigDiffedAgainstDefault(conf roachpb.SpanConfig) string { if conf.Equal(roachpb.TestingDefaultSystemSpanConfiguration()) { return "system target default"
nit: "default system span config"
pkg/ccl/spanconfigccl/spanconfigsqltranslatorccl/datadriven_test.go, line 163 at r2 (raw file):
sqlTranslator := tenant.SpanConfigSQLTranslator().(spanconfig.SQLTranslator) records, _, err := sqlTranslator.Translate(ctx, descpb.IDs{objID}, true /* generateSystemSpanConfigurations */)
Should we create a new directive to just generate system span configurations? As is, it's slightly confusing that these things are getting generated when translating a table or a database.
adityamaru
force-pushed
the
teach-translator
branch
2 times, most recently
from
258861b
to
da743ee
Compare
|
Addressed all your comments, TFTR! |
|
bors r=arulajmani |
…ired This change teaches the SQLTranslator to emit SpanConfigurations corresponding to `spanconfig.SystemTargets`. Today, these SpanConfigurations only contain ProtectionPolicies, corresponding to protected timestamp records that may be written by the host tenant to protect its cluster, a secondary tenant to protect its cluster, or the host tenant to protect a secondary tenant. The SystemTarget span configurations will be applied to a SystemSpanConfig store that will be introduced in a follow up PR. Informs: cockroachdb#73727 Release note: None
adityamaru
force-pushed
the
teach-translator
branch
from
da743ee
to
160fb19
Compare
|
Merge conflict, rebasing and trying again. |
|
bors r=arulajmani |
|
Build failed (retrying...): |
|
bors r+ |
|
Already running a review |
|
Build failed (retrying...): |
|
Build failed: |
|
Unrelated flake, lets try this again. bors r=arulajmani |
|
Build succeeded: |
This change teaches the SQLTranslator to emit SpanConfigurations
corresponding to
spanconfig.SystemTargets. Today, these SpanConfigurationsonly contain ProtectionPolicies, corresponding to protected timestamp
records that may be written by the host tenant to protect its cluster,
a secondary tenant to protect its cluster, or the host tenant to protect
a secondary tenant.
The SystemTarget span configurations will be applied to a SystemSpanConfig
store that will be introduced in a follow up PR.
Informs: #73727
Release note: None