CAMEL-24154: camel-aws2-ddb - Fix DDB Streams consumer silent data loss on resharding#24861
Conversation
…ss on resharding Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of gnodet
Excellent fix for four related data-loss bugs in ShardIteratorHandler — each targeting a distinct failure mode during DynamoDB shard resharding.
Changes verified:
-
Stale shard tree ✅
pendingClosedShardstracks shards removed viaupdateShardIterator(id, null)- On the next poll, if any pending closed shards exist,
describeStreamPaginated()refreshes the tree - Children of closed shards are discovered and initialized with
TRIM_HORIZONiterators pendingClosedShards.clear()after processing prevents redundant refreshes
-
ConcurrentModificationException ✅
return new HashMap<>(currentShardIterators)— defensive copy prevents the consumer's iteration loop from conflicting withupdateShardIterator()modifications- Test (
shouldReturnDefensiveCopyOfShardIterators) verifies mutation isolation
-
Missing
describeStreampagination ✅describeStreamPaginated()loops onlastEvaluatedShardId— correctly handles streams with >100 shards- The old code made a single call with no pagination, silently losing shards
-
Stuck consumer on expired iterator ✅
lastSeenSequenceNumber != nullguard selectsAFTER_SEQUENCE_NUMBERvsTRIM_HORIZONAFTER_SEQUENCE_NUMBERwith null sequence number would produce an API error, leaving the consumer stuck- Test (
shouldUseTrimHorizonWhenSequenceNumberIsNull) verifies the request capturesTRIM_HORIZON
Test mock improvements:
setMockedShardAndIteratorResponsenow removes existing entries byshardIdbefore adding — correctly models shard state transitions (open → closed)getShardIteratorRequests()exposes request tracking for iterator type assertions
Resharding test (shouldDiscoverNewShardsAfterResharding):
Well-structured simulation: two initial shards → shard A closes → A splits into A1/A2 → next poll discovers children. Verifies the final iterator map contains A1, A2, B (not A).
No issues found.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 tested, 29 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
…egistry lookup Restore the registry lookup in resolveResumeAction but create a per-shard instance via reflection to fix the thread-safety issue. This preserves backward compatibility for users who register custom KinesisResumeAction subclasses while avoiding concurrent mutation of a shared singleton. Also rebased onto main to incorporate PR #24861's DDB stream fixes, dropping the overlapping commit from this PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <davsclaus@apache.org> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
…egistry lookup Restore the registry lookup in resolveResumeAction but create a per-shard instance via reflection to fix the thread-safety issue. This preserves backward compatibility for users who register custom KinesisResumeAction subclasses while avoiding concurrent mutation of a shared singleton. Also rebased onto main to incorporate PR #24861's DDB stream fixes, dropping the overlapping commit from this PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <davsclaus@apache.org> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
…egistry lookup Restore the registry lookup in resolveResumeAction but create a per-shard instance via reflection to fix the thread-safety issue. This preserves backward compatibility for users who register custom KinesisResumeAction subclasses while avoiding concurrent mutation of a shared singleton. Also rebased onto main to incorporate PR #24861's DDB stream fixes, dropping the overlapping commit from this PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <davsclaus@apache.org> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
Claude Code on behalf of davsclaus
Summary
Fixes four related defects in
ShardIteratorHandlerthat cause silent data loss when DynamoDB rotates or splits shards (every ~4 hours):Stale shard tree —
shardTree.populate()was only called whencurrentShardIteratorswas empty. Shards created after startup were never discovered, causing all subsequent writes to a rotated partition to be silently dropped.ConcurrentModificationException —
getShardIterators()returned the internalHashMapby reference whileupdateShardIterator()structurally modified it during the consumer's iteration loop.Missing
describeStreampagination — streams with >100 shards lost partitions becauselastEvaluatedShardIdwas ignored.Stuck consumer on expired iterator —
requestFreshShardIteratorsentAFTER_SEQUENCE_NUMBERwith a null sequence number when the iterator expired before any record was read.Changes
ShardIteratorHandler: track closed shards inpendingClosedShards; refresh the tree via paginateddescribeStreamwhen a shard closes; return a defensive copy; fall back toTRIM_HORIZONon null sequence number.AmazonDDBStreamsClientMock: replace-by-shardId support and request tracking for test assertions.ShardIteratorHandlerTest: 3 new tests covering resharding discovery, defensive copy, and null-sequence fallback.Test plan
mvn testincamel-aws2-ddb)🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com