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

status: skip TestTenantStatusAPI/.../test_tenant_ranges_pagination #97386

Merged
merged 1 commit into from
Feb 23, 2023

Conversation

abarganier
Copy link
Contributor

@abarganier abarganier commented Feb 21, 2023

Refs: #92979

Reason: flaky test, difficult to reproduce locally. Skip until resolved.

Release note: none

Informs: #92979

@abarganier abarganier requested review from maryliag and a team February 21, 2023 15:31
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@abarganier abarganier marked this pull request as ready for review February 21, 2023 15:32
@abarganier abarganier requested a review from a team February 21, 2023 15:32
@abarganier abarganier requested a review from a team as a code owner February 21, 2023 15:32
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@dhartunian
Copy link
Collaborator

bors r=maryliag

@craig
Copy link
Contributor

craig bot commented Feb 21, 2023

Canceled.

Refs: cockroachdb#92979

Reason: flaky test, difficult to reproduce locally. Skip until resolved.

Release note: none
@abarganier
Copy link
Contributor Author

bors r= maryliag

@craig
Copy link
Contributor

craig bot commented Feb 23, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Feb 23, 2023

Build succeeded:

@craig craig bot merged commit 352cf35 into cockroachdb:master Feb 23, 2023
abarganier added a commit to abarganier/cockroach that referenced this pull request Mar 21, 2023
Fixes: cockroachdb#92979

Previously, in cockroachdb#97386,
we skipped test_tenant_ranges_pagination because it was marked as flaky.

The test makes a request for a single range and expects an offset of `1`
back. It then uses this offset to request a second range, and expects
an offset of `2`. This means that the test requires at least 3 ranges
to exist on the tenant.

The test was flaking on the assertion that the offset returned by the
second request came back as `2`. Instead, it was flaking when the
offset came back as `0`, which signifies that there are no more
ranges to process.

We learned that the tenant create process has an asycnhronous splitting
of ranges that occurs, which is what would lead to this sporadic scenario
where not enough ranges existed (yet) for the test to succeed.

This patch updates the test with a `testutils.SucceedsSoon` clause that
checks first that `crdb_internal.ranges` contains at least 3 ranges,
prior to making the second request. This should provide sufficient time
for the range split queue to be processed and eliminate the vast majority
of these test flakes.

Release note: none
craig bot pushed a commit that referenced this pull request Mar 21, 2023
98527: sql: deprecate gossip-based physical planning r=dt a=dt

Previously system tenants and secondary tenants used differnt physical planning implementations, with the system tenant and only the system tenant using nodeIDs while other tenants used the instance table. This unifies those implementations such that all tenants use NodeIDs if running in mixed mode and use the instance table if not.

Release note: none.
Epic: CRDB-16910

98879: sql: support array-flatten subqueries within UDFs r=mgartner a=mgartner

Array-flatten subqueries (e.g., `ARRAY(SELECT a FROM t)`) are now
supported within UDFs. They are now converted to a normal subquery with
a ScalarGroupBy if they exist within a UDF, even if they are
uncorrelated. This allows them to be executed without any changes to the
execbuilder or the evaluation logic of `tree.Routine`.

Fixes #98738

Release note: None


98988: ui: add badges for filter elements r=maryliag a=maryliag

Adds badges for each of the selected filters on SQL Activity and
Insights pages.

Part Of #98891

https://www.loom.com/share/e7417209fc704d71b2733f58609fb4de

<img width="1160" alt="Screenshot 2023-03-19 at 1 30 49 PM" src="https://user-images.githubusercontent.com/1017486/226195412-03d3ef58-5d6d-4c24-84f1-6a55b952f5c0.png">

Release note (ui change): Adds badges for each selected
filter on SQL Activity and Insights pages.

99042: sql: added indexes to system.statement_statistics, system.transaction_statistics r=ericharmeling a=ericharmeling

Fixes #98624.

This commit adds indexes on new computed columns to the system.statement_statistics and system.transaction_statistics tables.

Epic: none

Release note: None

99054: pkg/ccl: Unskip TestTenantStatusAPI/tenant_ranges/pagination r=dhartunian a=abarganier

Fixes: #92979

Previously, in #97386, we skipped test_tenant_ranges_pagination because it was marked as flaky.

The test makes a request for a single range and expects an offset of `1` back. It then uses this offset to request a second range, and expects an offset of `2`. This means that the test requires at least 3 ranges to exist on the tenant.

The test was flaking on the assertion that the offset returned by the second request came back as `2`. Instead, it was flaking when the offset came back as `0`, which signifies that there are no more ranges to process.

We learned that the tenant create process has an asycnhronous splitting of ranges that occurs, which is what would lead to this sporadic scenario where not enough ranges existed (yet) for the test to succeed.

This patch updates the test with a `testutils.SucceedsSoon` clause that checks first that `crdb_internal.ranges` contains at least 3 ranges, prior to making the second request. This should provide sufficient time for the range split queue to be processed and eliminate the vast majority of these test flakes.

Release note: none

99119: kvserver: mark in-flight storage checkpoints r=tbg a=pavelkalinnikov

This commit makes it so that the consistency checker checkpoints are first created in a "_pending" folder, and only after completion are atomically renamed to the intended directory name. This is to help distinguish valid checkpoints from the ones that weren't finalized (for example, when the node crashed in the meantime).

Part of #81819
Epic: none
Release note (ops change): checkpoint directories (that can be created in the rare event of range inconsistency) are now clearly indicated as pending until they are fully populated. This is to help operators to distinguish valid checkpoints from corrupted ones.

Co-authored-by: David Taylor <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: maryliag <[email protected]>
Co-authored-by: Eric Harmeling <[email protected]>
Co-authored-by: Alex Barganier <[email protected]>
Co-authored-by: Pavel Kalinnikov <[email protected]>
stevendanna pushed a commit to stevendanna/cockroach that referenced this pull request Jun 8, 2023
Fixes: cockroachdb#92979

Previously, in cockroachdb#97386,
we skipped test_tenant_ranges_pagination because it was marked as flaky.

The test makes a request for a single range and expects an offset of `1`
back. It then uses this offset to request a second range, and expects
an offset of `2`. This means that the test requires at least 3 ranges
to exist on the tenant.

The test was flaking on the assertion that the offset returned by the
second request came back as `2`. Instead, it was flaking when the
offset came back as `0`, which signifies that there are no more
ranges to process.

We learned that the tenant create process has an asycnhronous splitting
of ranges that occurs, which is what would lead to this sporadic scenario
where not enough ranges existed (yet) for the test to succeed.

This patch updates the test with a `testutils.SucceedsSoon` clause that
checks first that `crdb_internal.ranges` contains at least 3 ranges,
prior to making the second request. This should provide sufficient time
for the range split queue to be processed and eliminate the vast majority
of these test flakes.

Release note: none
zachlite pushed a commit to zachlite/cockroach that referenced this pull request Jul 11, 2023
Fixes: cockroachdb#92979

Previously, in cockroachdb#97386,
we skipped test_tenant_ranges_pagination because it was marked as flaky.

The test makes a request for a single range and expects an offset of `1`
back. It then uses this offset to request a second range, and expects
an offset of `2`. This means that the test requires at least 3 ranges
to exist on the tenant.

The test was flaking on the assertion that the offset returned by the
second request came back as `2`. Instead, it was flaking when the
offset came back as `0`, which signifies that there are no more
ranges to process.

We learned that the tenant create process has an asycnhronous splitting
of ranges that occurs, which is what would lead to this sporadic scenario
where not enough ranges existed (yet) for the test to succeed.

This patch updates the test with a `testutils.SucceedsSoon` clause that
checks first that `crdb_internal.ranges` contains at least 3 ranges,
prior to making the second request. This should provide sufficient time
for the range split queue to be processed and eliminate the vast majority
of these test flakes.

Release note: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants