scheduledlogging: fix TestCaptureIndexUsageStats for secondary tenants #109783
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 |
This commit fixes flaky behavior when TestCaptureIndexUsageStats is run for secondary tenants. The previous synchronization mechanism between logging and the test was not consistent between system and secondary tenants. Instead, there's no attempt to synchronize, and we rely on testutils.SucceedsSoon instead. Resolves cockroachdb#109465 Epic: none Release note: None
zachlite
force-pushed
the
230830.captured_index_usage_stats
branch
from
c13360c to
5add509
Compare
j82w
approved these changes
Aug 31, 2023
complete! 1 of 0 LGTMs obtained
zachlite
added
backport-23.1.x
|
bors r+ |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 5add509 to blathers/backport-release-23.1-109783: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
xinhaoz
added a commit
to xinhaoz/cockroach
that referenced
this pull request
Sep 7, 2023
This commit reverts cockroachdb#109783 which added a `SucceedsSoon` in place of testing each anticipated sheduling of index usage stats recording. Since this test checks the number of logs emitted after each schedule, adding `SucceedsSoon` meant that entering a retry loop could actually mean we fall into the next scheduled index usage stats emission, thus surpassing the expected number of logs for the schedule number being checked. This addition was meant to fix the test for secondary tenants by waiting for all schedules of tenants to run. This commit instead disables index usage stats for the system tenant, allowing us to once again synchronize a single tenant's schedule in order to verify the logs. Fixes: cockroachdb#109924
xinhaoz
added a commit
to xinhaoz/cockroach
that referenced
this pull request
Sep 8, 2023
This commit reverts cockroachdb#109783 which added a `SucceedsSoon` in place of testing each anticipated sheduling of index usage stats recording. Since this test checks the number of logs emitted after each schedule, adding `SucceedsSoon` meant that entering a retry loop could actually mean we fall into the next scheduled index usage stats emission, thus surpassing the expected number of logs for the schedule number being checked. This addition was meant to fix the test for secondary tenants by waiting for all schedules of tenants to run. This commit instead disables index usage stats for the system tenant, allowing us to once again synchronize a single tenant's schedule in order to verify the logs. Fixes: cockroachdb#109924 Release note: None
craig bot
pushed a commit
that referenced
this pull request
Sep 8, 2023
110224: scheduledlogging: fix `TestCaptureIndexUsageStats` r=xinhaoz a=xinhaoz This commit reverts #109783 which added a `SucceedsSoon` in place of testing each anticipated sheduling of index usage stats recording. Since this test checks the number of logs emitted after each schedule, adding `SucceedsSoon` meant that entering a retry loop could actually mean we fall into the next scheduled index usage stats emission, thus surpassing the expected number of logs for the schedule number being checked. This addition was meant to fix the test for secondary tenants by waiting for all schedules of tenants to run. This commit instead disables index usage stats for the system tenant, allowing us to once again synchronize a single tenant's schedule in order to verify the logs. Fixes: #109924 Release note: None Co-authored-by: Xin Hao Zhang <[email protected]>
xinhaoz
added a commit
to xinhaoz/cockroach
that referenced
this pull request
Sep 8, 2023
This commit reverts cockroachdb#109783 which added a `SucceedsSoon` in place of testing each anticipated sheduling of index usage stats recording. Since this test checks the number of logs emitted after each schedule, adding `SucceedsSoon` meant that entering a retry loop could actually mean we fall into the next scheduled index usage stats emission, thus surpassing the expected number of logs for the schedule number being checked. This addition was meant to fix the test for secondary tenants by waiting for all schedules of tenants to run. This commit instead disables index usage stats for the system tenant, allowing us to once again synchronize a single tenant's schedule in order to verify the logs. Fixes: cockroachdb#109924 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.
This commit fixes flaky behavior when TestCaptureIndexUsageStats is run for secondary tenants. The previous synchronization mechanism between logging and the test was not consistent between system and secondary tenants.
Instead, there's no attempt to synchronize, and we rely on testutils.SucceedsSoon instead.
Resolves #109465
Epic: none
Release note: None