Repository navigation
CON-599: aws_dynamodb_cdc: stop re-reading exhausted closed shards - #5001
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Connect docs checkThe docs build passed with this PR's generated docs. Preview: https://connect-pr-5001--redpanda-connect.netlify.app/connect/home/ This PR changes 1 published page (3 generated files: 3 changed, 0 added, 0 removed). Review the rendered pages, not only the Go strings.
Per-file diffs and the rendered-HTML findings are in the job summary. Checked at 1db8f68. |
Leward
left a comment
Leward
left a comment
There was a problem hiding this comment.
The PR body is out of date with the code at.
It says: "The set is in memory only, so a restart re-reads each retained closed shard once." However, the code now persists completion (MarkShardCompleted).
Please update the body so that reviewers check the persisted marker and these changes.
|
#5015 fixes a gap in the finalization this PR adds, not a bug on Non-blocking design point, for a follow-up PR. Checkpoint writes only run in reaction to an event (ack,
connect/internal/impl/aws/dynamodb/batcher.go Line 193 in c87547a Proposal: one persister per batcher. The tracker keeps the wanted state per shard (frontier, frontier time, completed) and the persisted state. One goroutine writes the difference. Acks and closes wake it, and a timer with backoff retries it. This removes |
|
Folded in: #5015's commit 21cdd6f is on this branch unchanged, and #5015 is closed out (it was based on this branch). I've also updated the PR body to cover the persisted completion marker, the RetryFinalize path and the global-mode changes. Agreed on the single persister per batcher. I'll open a ticket for it and link it here, as a separate PR since AckBatch no longer returning write errors needs its own review. |
Leward
left a comment
Leward
left a comment
There was a problem hiding this comment.
Requesting changes for one item: a child shard can wait for its parent shard for up to 24 hours, and no warning is logged. See the inline comment on parentDone.
The fix I ask for is small: log a warning when a child shard waits for its parent for longer than a limit. It can go in this PR or in a PR stacked on this branch, as #5015 was. I will approve once it is in this branch.
The other inline comments are non-blocking. The two global table fixes and the RetryFinalize retry look correct, and the new tests cover them.
Please also update the PR body. It still says "The set is in memory only, so a restart re-reads each retained closed shard once". That is no longer true: MarkShardCompleted persists a ShardCompleted marker, and a restart skips the shard:
The body also does not mention these changes:
- A child shard waits until its parent shard is read to the end.
- A trimmed checkpoint resumes from
TRIM_HORIZON. - A failed completion write is retried on each shard refresh (CON-601).
- The principal needs
dynamodb:UpdateItemon the checkpoint table.
|
Updated the PR description to cover the items listed in the review, please have another look when you get a chance. |
…parents, forget finished shard state
…HORIZON so it cannot stall its children
…oint position survives
The Checkpointing section now says the principal needs dynamodb:UpdateItem on the checkpoint table (MarkShardCompleted is an UpdateItem), and describes the behaviour this PR adds: closed shards read to the end are marked completed and skipped after a restart, a child shard is read only after its parent is read to the end (except on the first discovery under start_from: latest), and a checkpoint pointing at trimmed stream data resumes from TRIM_HORIZON with a warning. max_tracked_shards now says a shard is normally forgotten once read to the end and acknowledged.
…hildren A child shard was held back for as long as its parent stayed listed and unfinished, so a parent that could not be prepared for a reason other than trimmed data kept its whole lineage unread until it aged out of the listing (up to 24h). Discovery now remembers failed prepares across refresh cycles: a parent whose GetShardIterator returns ResourceNotFoundException is treated as done for ordering right away, and a parent that fails to prepare on 5 consecutive refresh cycles releases its children with a single warning. Neither writes a completion marker for the parent, which is still retried and read if it becomes readable.
GetShardIterator returns no iterator for a closed shard with nothing left to read, and an iterator refresh can do the same. The reader treated a nil iterator as "stop" and returned silently, leaving the shard registered but unread: it was never marked completed and its children stayed held back behind it. A registered, unfinished reader holding a nil iterator now takes the closed-shard path (completeShard / completeTableShard), which persists the completion marker and releases the children.
…utdown A failed write of a shard's completion marker was logged but not counted, so dynamodb_cdc_checkpoint_failures missed it. completeShard and completeTableShard now increment the metric when the write fails. A write that failed only because the reader's context was canceled (shutdown) is expected: it is logged at debug instead of error and not counted.
…rdinator Adds coverage for two paths the PR changed without a direct test: syncShardReaders restarting a reader that gave up and was re-registered under the same shard ID (the old goroutine is canceled and a fresh one is started for the new reader), and a multi-table variant of TestShardCoordinatorReadsClosedShardOnce driving the table stream coordinator. The in-memory checkpoint store shared by both coordinator tests moves into lockedStoreCheckpointer.
…ach shard refresh (CON-601)
…o time on failover
Leward
left a comment
Leward
left a comment
There was a problem hiding this comment.
The blocking item is in: a child shard held back behind its parent now logs a warning after 20 refresh cycles and again every 20, and the test covers both the single-table and multi-table coordinators. The iterator refreshes in both readers go through acquireShardIterator, the spec lists all three cases where a child is read before its parent, and ForgetResolved bounds the global-mode resolved set. The PR body now matches the code. Two non-blocking suggestions inline, both fine as follow-ups. Please link the single-persister ticket here when it exists.
@Leward the single-persister follow-up is CON-613: https://redpandadata.atlassian.net/browse/CON-613. It will be a separate PR, since AckBatch no longer returning write errors needs its own review. |
What
aws_dynamodb_cdc stops re-reading closed shards it has already read to the end, both within a run and across restarts.
Why
The 5-minute exhausted-shard cleanup dropped a finished reader, and the next shard refresh rediscovered the closed shard (still listed for up to 24h) and read it again from its lagging checkpoint, redelivering records. The final page of a closed shard (nil NextShardIterator) was also dropped.
How
completedShardsset (single-table input and eachtableStream) records shards read to the end;refreshShards/refreshTableShardsskip them, and entries are pruned once DescribeStream stops listing the shard. Records on the final nil-iterator page are delivered.RecordBatcher.CloseShardfinalizes a closed shard once its last batch settles. If every batch was acked,Checkpointer.MarkShardCompletedwrites the final position plus aShardCompletedmarker withUpdateItem, keeping the stored SequenceNumber and, in global mode, ApproximateCreationTime. If a batch was nacked, only the pinned frontier is written. A restart resolves a marked shard toresumeCompletedand skips it. This also persists the closed-shard tail, which covers most of CON-598.RetryFinalize, folded in from CON-601: aws_dynamodb_cdc: retry a failed shard completion write on refresh #5015) and counted incheckpoint_failures. A write canceled by shutdown is logged at debug.start_from: latestis still honored. A parent that is gone is treated as done, and one that fails to prepare 5 cycles in a row releases its children; the spec lists all three exceptions. A child still waiting after 20 refresh cycles (about 10 minutes) logs a warning naming the parent, repeated every 20 cycles. An exact resume that hits TrimmedDataAccessException falls back to TRIM_HORIZON.acquireShardIterator, so a hung GetShardIterator times out instead of stalling the shard and its children.dynamodb:UpdateItem.Review focus
The persisted completion marker (
MarkShardCompleted,ResolveResume/prepareResume, andCloseShard/finalizeShardin batcher.go), the parent-before-child gating in shard discovery, and the global-table resume paths.Testing
go test -race ./internal/impl/aws/...-tags integration) against dynamodb-localJira: https://redpandadata.atlassian.net/browse/CON-599