test: increase MV schema-notification wait budget in metadata tests - #1021
Open
mykaul wants to merge 2 commits into
Open
test: increase MV schema-notification wait budget in metadata tests#1021mykaul wants to merge 2 commits into
mykaul wants to merge 2 commits into
Conversation
Materialized-view schema-change notifications arrive as a separate control-connection event from the base-table schema response, so there is an inherent lag between a base-table ALTER completing and the MV's own metadata reflecting it. The existing workaround in test_base_table_column_addition_mv and test_base_table_type_alter_mv capped this wait at 10 x 0.2s = 2s, which is too tight under CI load and flakes intermittently. Replace both hand-rolled retry loops with the existing tests.util.wait_until helper, raising the budget to 60 x 0.5s = 30s. This reuses the codebase's standard wait-for-condition primitive instead of duplicating another ad hoc loop, and gives the control connection realistic headroom to deliver the MV notification. Also drops the now-unused 'import time' left behind by removing the last time.sleep() calls in this file. Fixes: scylladb#1020 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 37 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review by Qodo
1.
|
![qodo-scylladb[bot]](https://avatars.githubusercontent.com/u/14364730?s=60&v=4){kind=small}
… poll wait_until() checked condition() first, then looked at the *attempt* counter to decide whether to raise, instead of the condition's own last result. When the condition finally became true exactly on the poll after the last sleep (attempt == max_attempts), the loop correctly exited, but the post-loop 'if attempt >= max_attempts: raise' fired anyway, reporting a false timeout. This was latent before but surfaced by PR scylladb#1021 raising the metadata tests' max_attempts from 10 to 60: a wider window makes hitting that exact boundary far more likely under real CI load. Cache each condition() call's own result and check that instead of the attempt counter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Problem
test_base_table_column_addition_mvandtest_base_table_type_alter_mv(tests/integration/standard/test_metadata.py) intermittently fail in CI. Both already contain a self-documented workaround for a known issue: materialized-view schema-change notifications arrive as a separate control-connection event from the base-table schema response, so there's an inherent lag between a base-tableALTERcompleting and the MV's own metadata reflecting it.The existing retry loops capped this wait at 10 x 0.2s = 2s total, which is apparently not always enough under CI load, causing the final assertion to fail intermittently. This surfaced most recently as a seemingly-unrelated CI failure on PR #786 — that PR's own change was confirmed correct; the failure was this pre-existing flake.
Fix
Replace both hand-rolled
for i in range(10): ... time.sleep(.2)loops with the existingtests.util.wait_until(condition, delay, max_attempts)helper (already used elsewhere in this same file), raising the budget to 60 x 0.5s = 30s. This reuses the codebase's standard wait-for-condition primitive instead of duplicating another ad hoc loop, and gives the control connection realistic headroom to deliver the MV notification.Also removes the now-unused
import timeleft behind once the lasttime.sleep()calls in this file were removed.Grepped the file for any other occurrence of this same fragile pattern — these were the only two.
Fixes: #1020
Testing
ast.parse/ syntax check passes.alltimehigh,monthlyhigh) are already asserted present earlier in each test before the newwait_untilcalls, so the lambda's dict/key access can't raise before the condition is met.wait_until's contract (checks condition first, sleeps between attempts, raises on exhaustion) matches the intent, and mirrors existing usage elsewhere in the same file (line ~128).🤖 Generated with Claude Code