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

release-22.2: pkg/ccl: Unskip TestTenantStatusAPI/tenant_ranges/pagination #106632

Closed

Conversation

zachlite
Copy link
Contributor

Backport commits from #106631 and #99054


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


Release justification: flake test fix

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 zachlite requested review from a team July 11, 2023 21:54
@zachlite zachlite requested a review from a team as a code owner July 11, 2023 21:54
@zachlite
Copy link
Contributor Author

cc @abarganier

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@zachlite zachlite changed the title pkg/ccl: Unskip TestTenantStatusAPI/tenant_ranges/pagination release-22.2: pkg/ccl: Unskip TestTenantStatusAPI/tenant_ranges/pagination Jul 11, 2023
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

CI is trying to tell you something.

@zachlite
Copy link
Contributor Author

Closed in favor of #106710

@zachlite zachlite closed this Jul 12, 2023
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