changefeedccl: enable a couple of tests with test tenant #156306
Conversation
Release note: None
|
This change is |
| // Test uses SPLIT AT, which isn't currently supported for | ||
| // secondary tenants. Tracked with #76378. | ||
| DefaultTestTenant: base.TODOTestTenantDisabled, | ||
| DefaultTestTenant: base.TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet(142799), |
There was a problem hiding this comment.
The comments here are definitely stale, which is why I decided to remove them to avoid possible confusion. (In particular, when this comment was added, we needed to explicitly add tenantcapabilitiespb.CanAdminSplit to the tenant, but now this capability is granted by default, so no need to do anything about it. Perhaps the test will just work, perhaps it'll require some other adjustments, but SPLIT AT should just work.)
I'm working on doing first level triage of all things that reference #76378 with the goal of closing that issue soon. All tests / packages that have test tenant randomization disabled will have a separate, specific issue, and #142799 is the common issue for all CDC tests.
| const nodes = 4 | ||
| args := base.TestClusterArgs{ | ||
| ServerArgs: base.TestServerArgs{ | ||
| DefaultTestTenant: base.TODOTestTenantDisabled, // need nodelocal and splits. |
There was a problem hiding this comment.
This comment too, is it not accurate for why this test wasn't working for tenants?
There was a problem hiding this comment.
This comment is somewhat stale for sure. CanUseNodelocalStorage capability should now be granted in the tests by default (see testServer.grantDefaultTenantCapabilities).
|
|
||
| ctx := context.Background() | ||
| s, db, _ := serverutils.StartServer(t, base.TestServerArgs{ | ||
| // TODO(#98816): cdctest.GetHydratedTableDescriptor does not work with tenant dbs. |
There was a problem hiding this comment.
Awesome, nice that we have this test back.
|
Is the extended CI failure related? The unit test failure might be a flake. |
There was a problem hiding this comment.
TFTR!
Both essential CI and extended CI failures seem like flakes.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @aerfrei)
| const nodes = 4 | ||
| args := base.TestClusterArgs{ | ||
| ServerArgs: base.TestServerArgs{ | ||
| DefaultTestTenant: base.TODOTestTenantDisabled, // need nodelocal and splits. |
There was a problem hiding this comment.
This comment is somewhat stale for sure. CanUseNodelocalStorage capability should now be granted in the tests by default (see testServer.grantDefaultTenantCapabilities).
| // Test uses SPLIT AT, which isn't currently supported for | ||
| // secondary tenants. Tracked with #76378. | ||
| DefaultTestTenant: base.TODOTestTenantDisabled, | ||
| DefaultTestTenant: base.TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet(142799), |
There was a problem hiding this comment.
The comments here are definitely stale, which is why I decided to remove them to avoid possible confusion. (In particular, when this comment was added, we needed to explicitly add tenantcapabilitiespb.CanAdminSplit to the tenant, but now this capability is granted by default, so no need to do anything about it. Perhaps the test will just work, perhaps it'll require some other adjustments, but SPLIT AT should just work.)
I'm working on doing first level triage of all things that reference #76378 with the goal of closing that issue soon. All tests / packages that have test tenant randomization disabled will have a separate, specific issue, and #142799 is the common issue for all CDC tests.
|
TFTRs! bors r+ |
|
bors r+ |
156306: changefeedccl: enable a couple of tests with test tenant r=yuzefovich a=yuzefovich Fixes: #98816 Informs: #142799 Release note: None 156317: insights: skip contention test under duress r=alyshanjahani-crl a=dhartunian Resolves: #155838 Resolves: #155962 Epic: None Release note: None 156387: catalog/lease: address deadlock with locked lease timestamps r=fqazi a=fqazi Previously, when locked lease timestamps were used, we held the descriptor state while acquiring close timestamp handles. This could lead to a deadlock, since in other code paths we acquire the close timestamp handle first. To address this, this patch avoids holding the descriptor state lock longer than necessary. Fixes: #156177 Release note: None 156388: cliccl: fix FIPS-readiness check r=rail a=rickystewart The logic here is flipped. Release note: none Epic: DEVINF-1477 Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: David Hartunian <davidh@cockroachlabs.com> Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com> Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>
4 participants
Fixes: #98816
Informs: #142799
Release note: None