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

roachtest: cdc/mixed-versions support for shared-process deployments #128473

Conversation

renatolabs
Copy link
Contributor

This commit updates the cdc/mixed-versions test so that it is able to run on a shared-process deployment. Specifically, it updates the changefeed creator to take two connections: one to the tenant being tested (which could be the system tenant as well), and one to the system tenant. The latter is used to set cluster settings that are only visible to the system tenant and control how changefeeds work.

Informs: #127378

Release note: None

@renatolabs renatolabs requested a review from a team as a code owner August 7, 2024 16:33
@renatolabs renatolabs requested review from nameisbhaskar and DarrylWong and removed request for a team August 7, 2024 16:33
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@srosenberg srosenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@@ -401,6 +402,10 @@ func (cmvt *cdcMixedVersionTester) runWorkloadCmd(r *rand.Rand) *roachtestutil.C
func (cmvt *cdcMixedVersionTester) initWorkload(
ctx context.Context, l *logger.Logger, r *rand.Rand, h *mixedversion.Helper,
) error {
if err := enableTenantSplitScatter(l, r, h); err != nil {
Copy link
Member

@srosenberg srosenberg Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: seems arbitrary here, but I get why it's needed. Maybe the planner can automatically enable this for multi-tenant scenarios?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrmm, maybe, but some tests work fine without it -- it's only needed for tests that invoke workloads that perform split/scatter. For now I think having tests call this helper function explicitly is fine.

This commit updates the `cdc/mixed-versions` test so that it is able
to run on a shared-process deployment. Specifically, it updates the
changefeed creator to take two connections: one to the tenant being
tested (which could be the system tenant as well), and one to the
system tenant. The latter is used to set cluster settings that are
only visible to the system tenant and control how changefeeds work.

Informs: cockroachdb#127378

Release note: None
`enthropy` -> `entropy`

Epic: none

Release note: None
@renatolabs renatolabs force-pushed the rc/mixedversion-cdc-shared-process-support branch from 49c0d72 to 9afac09 Compare August 8, 2024 14:05
@renatolabs renatolabs added backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2 labels Aug 8, 2024
@renatolabs
Copy link
Contributor Author

TFTR!

bors r=srosenberg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x Flags PRs that need to be backported to 24.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants