improvement: targeted schema invalidation for type/function/aggregate events#885
improvement: targeted schema invalidation for type/function/aggregate events#885mykaul wants to merge 2 commits into
Conversation
caeebc7 to
d93a2bd
Compare
d93a2bd to
d14a0f0
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR changes type, function, and aggregate schema invalidation from whole-keyspace removal to per-category invalidation with generation tracking. Metadata refresh logic now preserves concurrent invalidations and performs targeted partial refreshes. Event handling and tests were updated to verify that keyspace entries remain cached and only affected system schema categories are queried. Sequence Diagram(s)sequenceDiagram
participant SchemaChangeEvent
participant handleSchemaEvent
participant metadataDescriber
participant getKeyspaceInternal
participant refreshPartialKeyspaceSchema
participant system_schema
SchemaChangeEvent->>handleSchemaEvent: Type/function/aggregate schema change
handleSchemaEvent->>metadataDescriber: Invalidate affected category
metadataDescriber->>getKeyspaceInternal: Mark cached metadata invalidated
getKeyspaceInternal->>refreshPartialKeyspaceSchema: Refresh stale categories
refreshPartialKeyspaceSchema->>system_schema: Query affected metadata
refreshPartialKeyspaceSchema-->>getKeyspaceInternal: Apply refreshed metadata
Suggested reviewers: 🚥 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 |
Expert correctness review — targeted schema invalidationPerformed a deep Go correctness review of this PR (518/-54 across 4 files). Focus areas: targeted invalidation correctness for TYPE/FUNCTION/AGGREGATE events, dependency completeness, concurrency on the schema cache, error handling, and edge cases. VerdictThe implementation is correct and well-engineered. No HIGH/MEDIUM correctness issues were found, so no code changes were required. Detailed analysis below. Correctness analysisTargeted invalidation does not miss dependent entries.
Concurrency / data races.
Error handling. Fetch/parse errors from Edge cases. Verified: event for an uncached keyspace is a safe no-op; DROP keyspace removes the entry entirely (recreated keyspace starts with clean flags); nil cache map handled in Minor (LOW, non-blocking, no change made)
Tests
RebaseCleanly rebased onto current BenchmarkingNot applicable — this is a correctness fix to schema-event invalidation, not a hot-path perf change. No benchmarks run. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
mykaul
changed the title
There was a problem hiding this comment.
Pull request overview
This PR improves schema-cache efficiency by introducing targeted invalidation and refresh for type/function/aggregate schema change events, avoiding expensive full keyspace reloads when only these lightweight schema objects change.
Changes:
- Add per-category dirty flags and monotonic generation counters for types, functions, and aggregates in
KeyspaceMetadata. - Implement partial keyspace refresh (
refreshPartialKeyspaceSchema) and integrate it intoGetKeyspaceflow so only invalidated categories are re-fetched. - Update schema event handling and tests to reflect targeted invalidation semantics (keyspace entry remains cached).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| metadata_scylla.go | Adds targeted invalidation flags/generations, atomic replace-with-merge on full refresh, and partial refresh logic for types/functions/aggregates. |
| metadata_scylla_test.go | Adds unit tests for the new invalidation methods, generation bumps, and cloning behavior. |
| events.go | Routes type/function/aggregate schema change events to targeted invalidation methods instead of clearing the entire keyspace cache. |
| events_unit_test.go | Updates schema event unit tests to expect keyspace retention and (for types) targeted refresh queries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ef6653d to
93f4086
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
metadata_scylla.go (1)
1079-1090: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFactor the aggregate function-linking into a shared helper. Both refresh paths currently resolve only
FinalFuncandStateFunc; keeping that logic in one place would avoid drift if the linking rules change later.🤖 Prompt for 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. In `@metadata_scylla.go` around lines 1079 - 1090, The aggregate function-linking logic in the metadata refresh path is duplicated and should be centralized. Move the resolution of AggregateMetadata.FinalFunc and AggregateMetadata.StateFunc into a shared helper used by the ks.Aggregates rebuild flow and the other refresh path, so both paths call the same linking logic from metadata_scylla.go and stay consistent if the rules change.
🤖 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.
Nitpick comments:
In `@metadata_scylla.go`:
- Around line 1079-1090: The aggregate function-linking logic in the metadata
refresh path is duplicated and should be centralized. Move the resolution of
AggregateMetadata.FinalFunc and AggregateMetadata.StateFunc into a shared helper
used by the ks.Aggregates rebuild flow and the other refresh path, so both paths
call the same linking logic from metadata_scylla.go and stay consistent if the
rules change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 68c829af-bd11-4c25-8cb2-5c95baabbd06
📒 Files selected for processing (4)
events.goevents_unit_test.gometadata_scylla.gometadata_scylla_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- events.go
- events_unit_test.go
- metadata_scylla_test.go
Previously, schema change events for types, functions, and aggregates invalidated the entire keyspace cache, forcing a full schema reload. This was unnecessary and expensive. Now each category has its own dirty flag and monotonic generation counter. Only the affected metadata (types, functions, aggregates) is re-fetched on the next GetKeyspace call, while tables, views, and indexes remain cached. Key design points: - Generation counters prevent clearing invalidations that arrived during an in-flight partial refresh. - Function invalidation cascades to aggregates since AggregateMetadata embeds FunctionMetadata by value. - Full keyspace refresh merges newer invalidation state to avoid dropping concurrent events. Fixes: scylladb#730
93f4086 to
033638c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
events_unit_test.go (1)
926-936: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnsure partial refresh completed and cleared invalidation flags correctly.
Consider adding
expectNoRequery: trueto this test case to verify that the partial refresh loop properly clears thetypesInvalidatedflag, preventing subsequent calls toGetTablefrom needlessly re-querying the system schema. This provides consistent behavioral coverage with theGetKeyspaceequivalent test case.💡 Proposed fix
{ name: "after type event: refreshes only types", knownKeyspaces: map[string][]tableInfo{ "test_ks": {{name: "tbl_a", columns: []columnInfo{{name: "id", kind: "partition_key", position: 0}}}}, }, populateKs: map[string][]string{"test_ks": {"tbl_a"}}, event: &frm.SchemaChangeType{Change: "CREATED", Keyspace: "test_ks", Object: "my_type"}, getTable: [2]string{"test_ks", "tbl_a"}, expectedQueries: map[string]int{ "SELECT * FROM system_schema.types WHERE keyspace_name = ?": 1, }, + expectNoRequery: true, },🤖 Prompt for 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. In `@events_unit_test.go` around lines 926 - 936, Update the “after type event: refreshes only types” test case to set expectNoRequery: true, ensuring the partial refresh clears typesInvalidated and subsequent GetTable calls do not re-query system_schema.types.
🤖 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 `@metadata_scylla.go`:
- Around line 937-954: Update the merge closure in the keyspace metadata refresh
flow to keep invalidation flags, generations, and their associated data from the
same source. When current.typesInvalidationGen, functionsInvalidationGen, or
aggregatesInvalidationGen is newer than the captured generation, copy the
corresponding current Types, Functions, or Aggregates along with its flag and
generation onto replacement, preserving lockstep data and metadata.
---
Nitpick comments:
In `@events_unit_test.go`:
- Around line 926-936: Update the “after type event: refreshes only types” test
case to set expectNoRequery: true, ensuring the partial refresh clears
typesInvalidated and subsequent GetTable calls do not re-query
system_schema.types.
🪄 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: 8eb05822-6d58-40e7-8831-09a67df68163
📒 Files selected for processing (4)
events.goevents_unit_test.gometadata_scylla.gometadata_scylla_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- events.go
73c56d0 to
7073a44
Compare
Previously, schema change events for types, functions, and aggregates invalidated the entire keyspace cache, forcing a full schema reload. This was unnecessary and expensive.
Now each category has its own dirty flag and monotonic generation counter. Only the affected metadata (types, functions, aggregates) is re-fetched on the next
GetKeyspacecall, while tables, views, and indexes remain cached.Key design points:
AggregateMetadataembedsFunctionMetadataby value.Fixes: #730