sql: flake in TestSerialNormalizationWithUniqueUnorderedID #91858
Assignees
Labels
A-sql-builtins
SQL built-in functions and semantics thereof.
C-test-failure
Broken test (automatically or manually discovered).
skipped-test
T-sql-foundations
SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Comments
knz
added
C-test-failure
blathers-crl
bot
added
the
T-sql-foundations
Merged
craig bot
pushed a commit
that referenced
this issue
Nov 14, 2022
91840: server: remove unused apiv2 server field r=knz a=dhartunian Epic: None Resolves: #91829 Release note: None 91857: skip flaky tests r=andrewbaptist a=knz Informs #91856 Informs #91858 Co-authored-by: David Hartunian <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]>
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this issue
Nov 14, 2022
The use of range stats and splits was not very stable and not needed. Fixes cockroachdb#91858 Release note: None
craig bot
pushed a commit
that referenced
this issue
Nov 22, 2022
91868: builtins: deflake TestSerialNormalizationWithUniqueUnorderedID r=ajwerner a=ajwerner The use of range stats and splits was not very stable and not needed. Fixes #91858 Release note: None 92157: ccl: make most CCL tests link in all the CCL code; don't skip tests r=andreimatei a=andreimatei Before this patch, a lot of ccl/... tests were being silently skipped 50% of the time, when `ShouldStartDefaultTestTenant()` decided to attempt to create a tenant: the `TestServer` startup code was trying to create a tenant which was failing in a lot of ccl test packages because the tests in these packages were running in binaries that didn't have various other ccl/... packages linked in. The tenant creation error was then swallowed and the test skipped. Having tests be skipped half the time is no good; this patch makes the tenant creation no longer fail for any ccl tests that's starting a server, and deletes the silent skipping. The transparent creation of the tenant is checking whether a license is active - which, in the context of tests, reduces to checking whether utilccl.TestingEnableEnterprise() was called (perhaps in main_test.go). However, this was not enough to guarantee that the tenant creation will succeed instead of failing with a bizarre "ccl binary required" error. Even though we knew that some CCL code was linked in (i.e. utilccl), this doesn't mean that *all* ccl code was linked in. This patch makes it such that a license check (i.e. utilccl.CheckEnterpriseEnabled()) doesn't succeed when "all the CCL code" (i.e. the `ccl` pkg) is not linked in. When that's not the case, the function panics, telling you that you need to add more deps to your test binary. The `TestingEnableEnterprise()` function moves `utilccl` to `ccl`, thus forcing the majority of ccl tests to import `ccl` (usually in their `main_test.go`, which generally moves to the `foo_test` pkg to avoid a dep cycle). I'm also leaving behind a utilccl.TestingEnableEnterpriseTricky(), which enables the license without forcing `ccl` to be imported. This is used by the few packages that want more control over enabling/disabling the license but cannot import `ccl` without a dep cycle; such tests in pkg `foo` generally import `ccl` from a file in the `foo_test` pkg (so `ccl` ends up being linked in the shared test binary). utilccl.CheckEnterpriseEnabled() stays in the `utilccl` pkg, so everyone can use it without dep cycles, but it relies from a bool injected from the `ccl` pkg. This patch builds on #92024, which made a bunch of ccl tests pass when running against a tenant, instead of fail, once they're no longer skipped. --- Now that utilccl.CheckEnterpriseEnabled() reliably determines whether the CCL features are around, it can be used from BSL code (through base.CheckEnterpriseEnabled()) to determine whether CCL features can be used. This is a corollary of the things that were going for the TestServer tenant creation, but this is how I actually got here: I tried to use follower reads, gated by base.CheckEnterpriseEnabled(), only to get an error in ccl tests that my binary is not ccl enough. Release note: None Epic: None Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: Andrei Matei <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Found here: https://teamcity.cockroachdb.com/buildConfiguration/Cockroach_BazelExtendedCi/7506505?showRootCauses=false&expandBuildChangesSection=true&expandBuildProblemsSection=true&expandBuildTestsSection=true
cc @rafiss for triage
Jira issue: CRDB-21460
The text was updated successfully, but these errors were encountered: