Conversation
There was a problem hiding this comment.
Pull request overview
Adds resilience to cqlengine schema management by retrying table metadata lookups after forcing a targeted metadata refresh, and introduces unit tests for the _get_table_metadata retry behavior.
Changes:
- Add refresh-and-retry handling for missing table metadata in
_sync_table. - Add refresh-and-retry handling for missing table metadata in
_get_table_metadata. - Add new unit tests covering
_get_table_metadatasuccess/refresh/failure paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
cassandra/cqlengine/management.py |
Retries table metadata lookup after cluster.refresh_table_metadata(...) in both _sync_table and _get_table_metadata. |
tests/unit/cqlengine/test_management.py |
New unit tests validating _get_table_metadata behavior when metadata is missing and becomes available (or stays missing). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6f1be93 to
d5c3f18
Compare
30a8fd7 to
59408e4
Compare
59408e4 to
0b48dd1
Compare
|
Warning Review limit reachedNext included review available in 59 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 (1)
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
Rebased onto current `master` (no conflicts; the only cqlengine-adjacent changes on master since this PR was branched — the SimpleStrategy→NetworkTopologyStrategy rename and an unused-imports cleanup — don't touch the modified lines). Re-verified the outstanding thread from @dkropachev: the test module docstring was fully genericized to Test results: Re: CI — all required checks (Docs build, Integration tests across libev/asyncio/asyncore, Python 3.11–3.14) pass. The "Test wheels building" workflow shows Force-pushed the rebased branch (same code changes, updated commit base) to keep the two logical commits intact per review history. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
tests/unit/cqlengine/test_management.py:157
- As above, this patch target does not exist because
issubclasscomes from builtins. The decorator therefore raisesAttributeErrorbefore the exception-propagation behavior is exercised.
@patch("cassandra.cqlengine.management.issubclass", return_value=True)
tests/unit/cqlengine/test_management.py:122
- This test never reaches
_sync_table:issubclassis resolved from Python builtins and is not an attribute ofcassandra.cqlengine.management, so this patch raisesAttributeErrorwhen the test starts. Create the module-level override explicitly (or use a realModelsubclass).
This issue also appears on line 157 of the same file.
@patch("cassandra.cqlengine.management.issubclass", return_value=True)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cassandra/cqlengine/management.py`:
- Around line 439-447: The refresh-and-lookup flow around
cluster.refresh_table_metadata must convert driver refresh failures into
contextual CQLEngineException errors, and the final metadata KeyError must be
re-raised with exception chaining via from exc. Update the affected
management.py block accordingly, and add a test in
tests/unit/cqlengine/test_management.py covering a refresh-time DriverException
and asserting the contextual CQLEngineException.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ec041138-c781-49bd-88a8-5cdaadefcc5d
📒 Files selected for processing (2)
cassandra/cqlengine/management.pytests/unit/cqlengine/test_management.py
0b48dd1 to
d028b2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/cqlengine/test_management.py`:
- Around line 67-79: The retry test around _get_table_metadata must make
metadata appear only as a consequence of refresh_table_metadata, not merely on
the second property access; update the refresh mock to mutate the mocked tables
state, and assert the existing refresh call. In the create-path test at
tests/unit/cqlengine/test_management.py:155-168, make execute() mutate metadata
similarly and assert execute() occurs before _get_table_metadata().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aec3857d-c8e0-4d4c-89a0-9246e4ffc409
📒 Files selected for processing (2)
cassandra/cqlengine/management.pytests/unit/cqlengine/test_management.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cassandra/cqlengine/management.py
d028b2b to
db3ea26
Compare
db3ea26 to
efa4dae
Compare
… sync_table After CREATE TABLE or ALTER TABLE, the local metadata cache may not yet contain the new table if schema agreement timed out or the automatic metadata refresh was skipped. _sync_table() and _get_table_metadata() unconditionally accessed cluster.metadata.keyspaces[ks].tables[table], which raised KeyError in this case. Wrap both lookups in try/except KeyError. On miss, force a targeted cluster.refresh_table_metadata() call and retry once. If the table is still not present after the forced refresh, raise a descriptive CQLEngineException instead of a bare KeyError. This follows the same defensive pattern already used in _sync_type(), which calls cluster.refresh_user_type_metadata() after CREATE TYPE. Add unit tests for _get_table_metadata verifying: immediate hit (no refresh), successful retry after refresh, and failure after refresh. Also wrap the refresh_table_metadata() call itself in try/except: it can raise a DriverException (e.g. if schema agreement fails), which was previously left to propagate uncaught. Catch it and re-raise as a contextual CQLEngineException, matching the exception path used for the "still missing after refresh" case. Chain both re-raises with `from exc` so the original driver error/KeyError is preserved in the traceback instead of being replaced by a bare raise. Add a unit test verifying that a DriverException raised during the refresh itself results in a CQLEngineException, not an unhandled driver error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Drop the detailed description from the test_management.py docstring, keeping only the general module-level summary.
efa4dae to
9b78589
Compare
… sync_table
After CREATE TABLE or ALTER TABLE, the local metadata cache may not yet contain the new table if schema agreement timed out or the automatic metadata refresh was skipped. _sync_table() and _get_table_metadata() unconditionally accessed cluster.metadata.keyspaces[ks].tables[table], which raised KeyError in this case.
Wrap both lookups in try/except KeyError. On miss, force a targeted cluster.refresh_table_metadata() call and retry once. If the table is still not present after the forced refresh, raise a descriptive CQLEngineException instead of a bare KeyError.
This follows the same defensive pattern already used in _sync_type(), which calls cluster.refresh_user_type_metadata() after CREATE TYPE.
Add unit tests for _get_table_metadata verifying: immediate hit (no refresh), successful retry after refresh, and failure after refresh.
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.