Skip to content

fix: DynamoDB CDC integration test timeout#4175

Open
squiidz wants to merge 1 commit intomainfrom
dynamodb_cdc_integration_test_timeout
Open

fix: DynamoDB CDC integration test timeout#4175
squiidz wants to merge 1 commit intomainfrom
dynamodb_cdc_integration_test_timeout

Conversation

@squiidz
Copy link
Copy Markdown
Contributor

@squiidz squiidz commented Mar 26, 2026

No description provided.

@squiidz squiidz force-pushed the dynamodb_cdc_integration_test_timeout branch from 9152ae6 to 0b5d4fc Compare March 26, 2026 17:21
@redpanda-data redpanda-data deleted a comment from claude bot Mar 26, 2026
@redpanda-data redpanda-data deleted a comment from claude bot Mar 26, 2026
@squiidz squiidz force-pushed the dynamodb_cdc_integration_test_timeout branch from 0b5d4fc to c94c471 Compare March 26, 2026 17:33
@claude
Copy link
Copy Markdown

claude bot commented Mar 26, 2026

Review

Commits
LGTM

Review
This PR fixes a race condition where writes between Connect() and async shard discovery could be missed (especially with start_from: latest), and adds handling for TrimmedDataAccessException by refreshing the shard iterator or marking the shard exhausted and signaling the coordinator. The implementation follows existing patterns (lock ordering, channel signaling, error classification) and the test changes improve reliability with context-based read timeouts.

LGTM

@redpanda-data redpanda-data deleted a comment from claude bot Mar 26, 2026
@redpanda-data redpanda-data deleted a comment from claude bot Mar 26, 2026
mmatczuk
mmatczuk reviewed Mar 27, 2026
View reviewed changes
internal/impl/aws/dynamodb/input_cdc.go
// Initialize message channel with buffer to reduce blocking between scanner and processor
// Buffer size of 1000 allows scanner to work ahead without blocking
d.msgChan = make(chan asyncMessage, 1000)
d.shardRefreshCh = make(chan struct{}, 1)
Copy link
Copy Markdown
Contributor

@mmatczuk mmatczuk Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder should this be 1

Copy link
Copy Markdown
Contributor Author

@squiidz squiidz Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has to be 1, senders do a non-blocking send (select/default), so unbuffered would lose the signal. More than 1 doesn't help since one refresh covers everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mmatczuk mmatczuk mmatczuk left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@squiidz @mmatczuk