schemastore: update TableInfo for surviving partitions - #5964
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughPartition DDL handlers now propagate updated ChangesPartition DDL propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant applyDDL
participant applyDDLFromPersistStorage
participant extractTableInfoFunc
participant versionedTableInfoStore
applyDDL->>versionedTableInfoStore: Apply updated TableInfo to current partition IDs
applyDDLFromPersistStorage->>extractTableInfoFunc: Extract TableInfo for current partition IDs
extractTableInfoFunc->>versionedTableInfoStore: Return updated or deleted partition state
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
ti-chi-bot
Bot
added
the
size/L
ti-chi-bot
Bot
removed
do-not-merge/work-in-progress
![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 `@logservice/schemastore/multi_version_test.go`:
- Around line 327-351: Expand the test around iterateEventTablesFunc and
updateDDLHistoryFunc to validate every post-DDL partition, not only survivorID.
Collect all table IDs passed to iterateEventTablesFunc and assert they match the
expected set of 201, 203, 204, and 205; also assert tablesDDLHistory contains
ddlFinishedTS for each expected ID before continuing with the store assertions.
🪄 Autofix
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: 6abb1a96-e3a3-486d-b286-035382680ddd
📒 Files selected for processing (2)
logservice/schemastore/multi_version_test.gologservice/schemastore/persist_storage_ddl_handlers.go
| // Verify that the online path applies the DDL to a registered surviving partition. | ||
| liveStore := newStore(true) | ||
| applied := false | ||
| handler.iterateEventTablesFunc(event, func(tableIDs ...int64) { | ||
| for _, tableID := range tableIDs { | ||
| if tableID == survivorID { | ||
| liveStore.applyDDL(event) | ||
| applied = true | ||
| } | ||
| } | ||
| }) | ||
| require.True(t, applied) | ||
| assertUpdated(liveStore) | ||
|
|
||
| // Verify that the history path records and extracts the same update. | ||
| tablesDDLHistory := make(map[int64][]uint64) | ||
| handler.updateDDLHistoryFunc(updateDDLHistoryFuncArgs{ | ||
| ddlEvent: event, | ||
| tablesDDLHistory: tablesDDLHistory, | ||
| }) | ||
| require.Equal(t, []uint64{ddlFinishedTS}, tablesDDLHistory[survivorID]) | ||
| historyStore := newStore(false) | ||
| historyStore.applyDDLFromPersistStorage(event) | ||
| historyStore.setTableInfoInitialized() | ||
| assertUpdated(historyStore) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert every affected partition ID.
The test validates only survivorID 201. It does not validate new partition IDs 203, 204, and 205.
A handler that skips newly created partitions can pass this test. Collect all IDs passed to iterateEventTablesFunc and assert the expected affected-ID set. Also assert tablesDDLHistory for each expected ID.
The PR objective requires updates for all post-DDL partitions.
🤖 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 `@logservice/schemastore/multi_version_test.go` around lines 327 - 351, Expand
the test around iterateEventTablesFunc and updateDDLHistoryFunc to validate
every post-DDL partition, not only survivorID. Collect all table IDs passed to
iterateEventTablesFunc and assert they match the expected set of 201, 203, 204,
and 205; also assert tablesDDLHistory contains ddlFinishedTS for each expected
ID before continuing with the store assertions.
1 participant
What problem does this PR solve?
Issue Number: close #5965
What is changed and how it works?
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note
Summary by CodeRabbit