Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

streamingest: TestRandomClientGeneration should use the proper SQLCodec in assertEqualKVs() #82883

Closed
knz opened this issue Jun 14, 2022 · 5 comments
Labels
A-multitenancy Related to multi-tenancy A-tenant-streaming Including cluster streaming A-testing Testing tools and infrastructure C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. E-quick-win Likely to be a quick win for someone experienced.
Milestone

Comments

@knz
Copy link
Contributor

knz commented Jun 14, 2022

The test TestRandomClientGeneration in streamingest/stream_ingestion_processor_test.go currently uses keys.TODOSQLCodec() in its post-conditiopn assertEqualKVs().

Instead this should be modified to use the codec expected for the replication stream being validated.

cc @stevendanna for triage.

Jira issue: CRDB-16710

@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-testing Testing tools and infrastructure A-multitenancy Related to multi-tenancy A-tenant-streaming Including cluster streaming labels Jun 14, 2022
@knz
Copy link
Contributor Author

knz commented Jun 14, 2022

@stevendanna could you help me understand what this test does? Would it make sense to modify the test to exercise both streaming from/into system tenant and a secondary tenant keyspace?

@knz knz added the E-quick-win Likely to be a quick win for someone experienced. label Jun 14, 2022
@stevendanna
Copy link
Collaborator

Would it make sense to modify the test to exercise both streaming from/into system tenant and a secondary tenant keyspace?

At the moment, the tenant streaming doesn't support the source or destination being the system tenant.

@knz
Copy link
Contributor Author

knz commented Jun 14, 2022

Ok so this makes fixing this issue easier: the SQLCodec should be the one for the tenant of the stream.

@shermanCRL shermanCRL added A-tenant-streaming Including cluster streaming and removed A-tenant-streaming Including cluster streaming labels Jun 23, 2022
@shermanCRL shermanCRL added this to the 22.2 milestone Jul 7, 2022
@yuzefovich
Copy link
Member

Should this issue be closed now? The test now correctly uses SQLCodec for the tenant. If we plan to support C2C for system tenants in the future, I'd argue we don't need this issue to track extension to this particular test; instead, it should be a part of the PR that introduces that new behavior.

@knz
Copy link
Contributor Author

knz commented Jun 20, 2023

yep, that works thanks

@knz knz closed this as completed Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy A-tenant-streaming Including cluster streaming A-testing Testing tools and infrastructure C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. E-quick-win Likely to be a quick win for someone experienced.
Projects
None yet
Development

No branches or pull requests

4 participants