pkg/ccl: Unskip TestTenantStatusAPI/tenant_ranges/pagination #99054
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
abarganier
force-pushed
the
ranges-test-fix
branch
from
6457fe8 to
0c69318
Compare
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
dhartunian
approved these changes
Mar 21, 2023
|
TFTR! bors r=dhartunian |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
zachlite
added a commit
to zachlite/cockroach
that referenced
this pull request
Jul 11, 2023
This test was mistakenly skipped in cockroachdb#105197, but the flake was addressed in cockroachdb#99054. The flake on the 22.2 release branch (tracked in cockroachdb#92382) was not novel, but un-treated. I misinterpreted this, and thought the skip on master would be for good measure, not realizing the flake was already treated. Epic: none Release note: None
This was referenced Jul 11, 2023
craig bot
pushed a commit
that referenced
this pull request
Jul 11, 2023
106631: statusccl: unskip testTenantRanges pagination r=zachlite a=zachlite This test was mistakenly skipped in #105197, but the flake was addressed in #99054. The flake on the 22.2 release branch (tracked in #92382) was not novel, but untreated. I misinterpreted this, and thought the skip on master would be for good measure, not realizing the flake was already treated. Epic: none Release note: None Co-authored-by: zachlite <[email protected]>
blathers-crl bot
pushed a commit
that referenced
this pull request
Jul 11, 2023
This test was mistakenly skipped in #105197, but the flake was addressed in #99054. The flake on the 22.2 release branch (tracked in #92382) was not novel, but un-treated. I misinterpreted this, and thought the skip on master would be for good measure, not realizing the flake was already treated. Epic: none Release note: None
zachlite
added a commit
to zachlite/cockroach
that referenced
this pull request
Jul 12, 2023
This is a 22.2 compatible work around for the flake fix introduced in cockroachdb#99054. In 22.2, crdb_internal.ranges is not yet compatible with secondary tenants, so instead we rely on TenantRanges itself with the `limit` param omitted. The method is the same: we count the number of ranges that belong to the tenant and only proceed when we know there are enough ranges for subsequent calls with pagination to succeed. Resolves cockroachdb#92382 Epic: none Release note: None
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
1back. It then uses this offset to request a second range, and expects an offset of2. 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 as0, 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.SucceedsSoonclause that checks first thatcrdb_internal.rangescontains 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