multiregionccl: add zone config waiting for partitioned tables#68444
multiregionccl: add zone config waiting for partitioned tables#68444craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
arulajmani
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @pawalt)
pkg/ccl/multiregionccl/datadriven_test.go, line 681 at r1 (raw file):
var lookupKey roachpb.Key if !d.HasArg(partitionName) {
nit: return early if there's no partitionName arg so you can get rid of the else case here.
pkg/testutils/serverutils/test_cluster_shim.go, line 197 at r1 (raw file):
// LookupTable returns the table descriptor for the specified database and // table names. LookupTable(database, table string) (catalog.TableDescriptor, error)
I don't think this should be an interface method on TestClusterInterface, mostly because Tables are a catalog concept and all the other interface methods here seem to be a function of the cluster/server.
Looks like the only thing that LookupTable needs from the test cluster is an executor config. I'd recommend passing that into the function instead of making it a method on TestCluster below.
As for where this should live, maybe we need a testutils file in the descs package which exposes a LookupHydratedTableDescriptor. I'm not sure, maybe @ajwerner might have a suggestion.
Alternatively, considering there's only one use of this method right now, you could move it into the file that you're calling it from for now as well.
pawalt
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/testutils/serverutils/test_cluster_shim.go, line 197 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
I don't think this should be an interface method on
TestClusterInterface, mostly because Tables are a catalog concept and all the other interface methods here seem to be a function of the cluster/server.Looks like the only thing that
LookupTableneeds from the test cluster is an executor config. I'd recommend passing that into the function instead of making it a method onTestClusterbelow.As for where this should live, maybe we need a
testutilsfile in the descs package which exposes aLookupHydratedTableDescriptor. I'm not sure, maybe @ajwerner might have a suggestion.Alternatively, considering there's only one use of this method right now, you could move it into the file that you're calling it from for now as well.
I've moved this to a helper function within the test file
d10db7b to
7628c95
Compare
arulajmani
left a comment
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3.
Reviewable status:
pkg/ccl/multiregionccl/datadriven_test.go, line 290 at r3 (raw file):
// wrapped in a succeeds soon. if err := testutils.SucceedsSoonError(func() error { lookupKey, err := getRangeKeyForInput(t, d, ds.tc)
Any reason this is inside the succeeds soon? Can we pull it out so we fail quickly in case some arguments aren't provided?
Previously, wait-for-zone-configs could only wait on tables, meaning we couldn't wait for zone config changes to apply to REGIONAL BY ROW partitions. This PR adds a `partition-name` flag to allow users to wait on a specific partition. Release note: None
pawalt
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/multiregionccl/datadriven_test.go, line 290 at r3 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Any reason this is inside the succeeds soon? Can we pull it out so we fail quickly in case some arguments aren't provided?
Good point; no reason why this should fail. I'll move it out.
|
bors r=arulajmani |
|
Build succeeded: |
Previously, wait-for-zone-configs could only wait on tables, meaning we
couldn't wait for zone config changes to apply to REGIONAL BY ROW
partitions. This PR adds a
partition-nameflag to allow users to waiton a specific partition.
Release note: None
A future PR will extend the tracing to allow us to trace REGIONAL BY ROW queries, but those changes combined with these will likely be too large for a single PR.