aws_dynamodb_batcher: encapsulate throttling logic in pinClock - #4749
aws_dynamodb_batcher: encapsulate throttling logic in pinClock#4749Leward wants to merge 2 commits into
pinClock#4749Conversation
Introduced the `pinClock` struct to standardize throttling duration tracking and warning rate-limiting for global and per-shard logic. Updated related tests accordingly.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| at: now.Add(5*time.Minute - time.Second), | ||
| wantSince: 5*time.Minute - time.Second, | ||
| wantWarn: false, | ||
| }, | ||
| { | ||
| name: "check at warn threshold triggers warning", | ||
| at: now.Add(5 * time.Minute), | ||
| wantSince: 5 * time.Minute, | ||
| wantWarn: true, | ||
| }, | ||
| { | ||
| name: "immediate check within interval is rate-limited", | ||
| at: now.Add(5*time.Minute + time.Second), | ||
| wantSince: 5*time.Minute + time.Second, | ||
| wantWarn: false, | ||
| }, | ||
| { | ||
| name: "check after interval triggers next warning", | ||
| at: now.Add(10 * time.Minute), | ||
| wantSince: 10 * time.Minute, |
There was a problem hiding this comment.
The new table hardcodes 5 * time.Minute / 10 * time.Minute for the warn threshold and the rate-limit interval instead of deriving them from the package constants throttlePinWarnAfter and throttlePinWarnInterval (batcher.go#L29-L32), which the neighbouring tests in this same file already use (batcher_test.go#L676-L678).
This violates the project Go style rule in CLAUDE.md → godev Magic Numbers: "Name all numeric constants. Every literal number in logic must have a clear meaning through a named constant or variable."
It also makes the test silently stop asserting what it claims: if throttlePinWarnAfter is lowered to, say, 2 minutes, the "check before warn threshold returns duration without warning" case at now+4m59s would then be past the threshold and the case would no longer be testing the sub-threshold path (it would fail or pass for the wrong reason depending on the interval).
Suggested fix: express the at/wantSince values in terms of throttlePinWarnAfter and throttlePinWarnInterval (e.g. throttlePinWarnAfter - time.Second, throttlePinWarnAfter, throttlePinWarnAfter + throttlePinWarnInterval) so the table tracks the constants.
| } | ||
|
|
||
| // check records the start of a throttling event or checks if a warning is due. | ||
| // Returns the total duration spent continuously throttled and whether a log warning should be emitted. |
There was a problem hiding this comment.
Godoc line exceeds the 80-character wrap limit (minor)
This line is ~103 characters. The project Go patterns (.claude/agents/godev.md, Documentation section) state: "Godoc must wrap at 80 characters per line." Every other comment block in this file wraps at ~75 columns, including the new pinClock field docs directly above.
Suggested fix: re-wrap the // Returns the total duration… sentence across two lines so it stays under 80 columns.
Context:
connect/internal/impl/aws/dynamodb/batcher.go
Lines 50 to 52 in 5091226
1 participant
Introduced the
pinClockstruct to standardize throttling duration tracking and warning rate-limiting for global and per-shard logic. Updated related tests accordingly.