aws_dynamodb_cdc: support DynamoDB Global Tables for the checkpoint t… - #4529
Conversation
|
Commits Review The change is well-structured and follows project patterns: the LGTM |
b957474 to
1ac155d
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…able Add `global_table` / `global_table_replicas` options so the auto-created checkpoint table is provisioned as a DynamoDB Global Table (v2), replicating shard checkpoints across regions for low-RPO multi-region failover. Checkpoints are keyed by a region-portable identifier (the source table name) and store the stream ARN and record timestamp. Resume is layered: same-region restarts resume exactly via AfterSequenceNumber (unchanged behavior), while a failed-over region resumes from the trim horizon, skipping records at or before the low-water-mark timestamp of the prior region's checkpoints. Table creation enables DynamoDB Streams and provisions/reconciles replica regions; all new behavior is gated behind `global_table` and is a no-op when disabled.
Addresses review feedback: the global_table field descriptions claimed the option only affects table creation and is a no-op on an existing table, but ensureTableExists reconciles replicas (issuing UpdateTable) on the existing-table path in global mode. Worse, a pre-existing non-global checkpoint table uses a StreamArn hash key that is incompatible with the global-mode TableId key schema, so reusing it would reconcile replicas onto the wrong table and write checkpoints under an undefined key. - Add validateGlobalTableSchema: when the checkpoint table already exists and global_table is enabled, require its hash key to be TableId and fail fast with an actionable error otherwise, before any UpdateTable. - Introduce key-attribute name constants so the create path, the key schema check, and checkpointKey can't drift apart. - Align the global_table / global_table_replicas descriptions with the reconcile-on-existing behavior and the global-mode schema requirement.
fb98f70 to
543dc7c
Compare
|
Commits Review LGTM |
isCDCCheckpointStale used Checkpointer.Get() and probed the returned sequence number with GetShardIterator against the current region's stream. In global mode after a regional failover, the checkpoint row (keyed by TableId) holds another region's sequence number, which is invalid against this region's stream, so the probe errored and the checkpoint was misclassified as stale — clearing the snapshot marker and forcing a full re-snapshot on the first restart after failover. That breaks the exact scenario the global-table feature exists for, with no user workaround. Use the global-table-aware ResolveResume() instead and only probe for staleness on an exact, same-region resume; failover and default resumes read from the trim horizon and are never stale.
|
Commits Review LGTM |
| for _, record := range records { | ||
| // Global-table failover: skip records already processed in the prior region. | ||
| if !failoverCutoff.IsZero() && record.Dynamodb != nil && record.Dynamodb.ApproximateCreationDateTime != nil { | ||
| if !record.Dynamodb.ApproximateCreationDateTime.After(failoverCutoff) { |
There was a problem hiding this comment.
Failover skip can drop unprocessed records, violating at-least-once (§5.4.2).
The cutoff is the minimum ApproximateCreationTime across the prior region's foreign checkpoint rows (prepareResume in checkpoint.go: if !cutoffSet || ts.Before(c.cutoff)). Here records are skipped when !After(cutoff), i.e. record.time <= cutoff.
DynamoDB Streams ApproximateCreationDateTime is second-granularity by default, so multiple records commonly share one timestamp. On the shard whose checkpoint is the minimum (the cutoff), records in that same second that occur after the checkpointed position were never processed in the prior region, yet <= cutoff skips them on resume — those records are silently dropped. That breaks the at-least-once guarantee the feature documents ("at-least-once, replaying from the trim horizon up to the last replicated record time").
Skipping strictly-before the cutoff (record.time.Before(cutoff)) is the at-least-once-safe choice: it reprocesses the boundary second (acceptable duplicates) rather than dropping it. The same issue exists in convertRecordsToBatch at line 2813.
See CONTRIBUTING §5.4.2 (At-least-once delivery). The boundary case is also untested — TestConvertTableRecordsToBatch_FailoverSkipsOldRecords only exercises minute-apart records.
|
Commits
The remaining three commits follow the Review The Global Tables checkpoint feature is well-structured (portable
|
The global-table failover replay skipped records with ApproximateCreationDateTime <= cutoff, where cutoff is the minimum ApproximateCreationTime across the prior region's checkpoints. Because that timestamp is only second-granular, records sharing the cutoff second on the min-watermark shard may have followed the checkpointed position and never been processed in the prior region — skipping them dropped data and broke the documented at-least-once guarantee (CONTRIBUTING §5.4.2). Skip only records strictly before the cutoff, replaying the boundary second (acceptable duplicates under at-least-once) instead of dropping it. Extract the decision into a single shouldSkipFailoverRecord helper used by both convert paths (previously duplicated) and cover the boundary second with unit tests.
|
Commits
The other four commits are well-scoped and correctly formatted. Review This is an incremental feature on the existing LGTM — no code-level issues found that warrant blocking. |
2 participants
aws_dynamodb_cdc: support DynamoDB Global Tables for the checkpoint table