feat(sqs): per-key dispatch helpers for partitioned-FIFO routing (Phase 3.D PR 5b-1)#731
feat(sqs): per-key dispatch helpers for partitioned-FIFO routing (Phase 3.D PR 5b-1)#731
Conversation
…se 3.D PR 5b-1) Foundation for PR 5b's Send/Receive partition fanout. Adds sqsMsg*KeyDispatch wrappers that route to the legacy or partitioned constructor based on meta.PartitionCount. Pure scaffold: helpers are added but no production call site uses them yet — those switch in PR 5b-2. What's added adapter/sqs_keys_dispatch.go (new file): - sqsMsgDataKeyDispatch - sqsMsgVisKeyDispatch - sqsMsgDedupKeyDispatch - sqsMsgGroupKeyDispatch - sqsMsgByAgeKeyDispatch - sqsMsgVisPrefixForQueueDispatch - effectivePartitionCount helper for fanout iteration Each dispatch helper takes (meta, queueName, partition, gen, …) and routes to the legacy single-partition constructor when meta.PartitionCount <= 1 (existing behaviour, byte-identical output) or to the partitioned constructor when > 1 (PR #715's keyspace). Why a separate scaffold PR PR 5b's full scope (Send/Receive fanout + receipt-handle v2 wiring + dormancy gate-lift + CreateQueue capability check) is ~1000 LOC across multiple files. The lessons-learned discipline from the recent rounds (PR #723 missed the raftadmin stub because the audit was function-scoped, not interface-scoped) favours smaller diffs. Splitting PR 5b into three: - PR 5b-1 (this PR): dispatch helpers + tests, zero behaviour change. - PR 5b-2: SendMessage / ReceiveMessage / DeleteMessage / ChangeMessageVisibility wire through the helpers. Receipt- handle v2 dispatch wired. Still gated by §11 PR 2 dormancy. - PR 5b-3 (atomic): gate-lift + CreateQueue capability check via PollSQSHTFIFOCapability. Small-but-critical. What does NOT change yet - 14 call sites in adapter/sqs_messages.go still call the legacy sqsMsg*Key constructors directly. PR 5b-2 swaps them. - encodeReceiptHandleV2 stays dormant. - decodeClientReceiptHandle still rejects v2 unconditionally. - CreateQueue still rejects PartitionCount > 1 with InvalidAttributeValue. Tests (5 top-level) - TestSQSKeysDispatch_LegacyMatchesLegacyConstructor: 8 sub-cases pinning byte-identical output for nil meta, PartitionCount=0, PartitionCount=1. - TestSQSKeysDispatch_PartitionedMatchesPartitionedConstructor: 6 sub-cases pinning byte-identical output for PartitionCount=4 partitioned constructors. - TestSQSKeysDispatch_LegacyAndPartitionedAreDistinct: keyspace isolation invariant — same logical record on legacy vs partitioned never shares a byte sequence. - TestEffectivePartitionCount: iteration-count contract (nil/0/1 → 1; explicit count otherwise). - TestSQSKeysDispatch_PartitionIgnoredOnLegacy: stale- partition caller against a non-partitioned queue produces the same key — protects against silent strand of messages. Self-review (per CLAUDE.md) 1. Data loss — wrappers only; no FSM/Pebble/retention path. Byte- identical output to legacy on non-partitioned queues. No issue. 2. Concurrency — pure functions, no shared state. No issue. 3. Performance — one nil-check + one comparison per dispatch. The non-partitioned path (steady state today) goes through the same constructor it always did. No issue. 4. Data consistency — keyspace isolation is pinned by TestSQSKeysDispatch_LegacyAndPartitionedAreDistinct. Stale- partition stranding is pinned by TestSQSKeysDispatch_PartitionIgnoredOnLegacy. No issue. 5. Test coverage — 19 sub-tests across the contract surface. Existing partitioned-constructor tests (PR #703) and legacy constructor tests continue to pass unchanged.
|
@claude review Phase 3.D PR 5b-1 — pure-additive dispatch helpers for partitioned-FIFO routing. Foundation for PR 5b-2's send/receive/delete wiring; PR 5b-3 then lifts the §11 PR 2 dormancy gate atomically with the CreateQueue capability check. No behaviour change in this PR — the 14 production call sites still use the legacy constructors directly. PR description has the rationale for the 3-way split and the full self-review. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces dispatch helper functions that route SQS key construction to either legacy single-partition or partitioned variants based on queue metadata. It includes an ChangesSQS Key Dispatch Helpers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 51 minutes and 54 seconds.Comment |
|
Claude finished @bootjp's task in 2m 29s —— View job Review of PR #731 — SQS per-key dispatch helpers (Phase 3.D PR 5b-1)Todo
OverallClean scaffold. The dispatch helpers are minimal and correct: all 6 share the same guard ( Issues1. PR description overstates nil/PartitionCount=0 coverageThe PR description says:
Looking at the actual test ( Since all 6 guards are currently identical this is not a correctness risk today, but the gap matters for ratchet value and the description creates a false impression of coverage. Suggest either:
2. No boundary test at PartitionCount=2 (the minimal partitioned value)
Observations (no action required)
Partition arg is silently dropped on legacy path. This is the correct design (documented in the file header), but PR 5b-2 callers should be audited: any path that constructs SummaryTwo suggested improvements before merge:
Neither is a correctness defect in the scaffold itself. The implementation is sound and the split strategy is appropriate. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08968f9361
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if meta == nil || meta.PartitionCount <= 1 { | ||
| return 1 | ||
| } | ||
| return meta.PartitionCount |
There was a problem hiding this comment.
effectivePartitionCount returns meta.PartitionCount for every queue with PartitionCount > 1, but partitionFor explicitly forces all message groups to partition 0 when FifoThroughputLimit == "perQueue" (adapter/sqs_partitioning.go). In that valid configuration, a Receive fanout built on this helper will scan empty partitions on every poll, multiplying read/CPU work by up to the configured partition count (max 32) with no correctness benefit. The helper should collapse to 1 partition in perQueue mode to match the routing behavior.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a central abstraction for SQS key dispatching, routing between legacy single-partition and new partitioned-FIFO layouts based on the queue's partition count. It includes several dispatch helpers for data, visibility, deduplication, and group keys, along with comprehensive unit tests to ensure backward compatibility and keyspace isolation. I have no feedback to provide.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
adapter/sqs_keys_dispatch_test.go (1)
15-63: 💤 Low valueEager key construction in table rows is slightly atypical — consider moving calls inside closures.
Both table-driven tests build
dispatchedandlegacy/partitionedvalues at slice-literal initialization time (before any sub-test runs), so a panic in a key constructor surfaces as a failure of the parent function rather than the named sub-test. This also means thet.Parallel()race detector cannot observe the key-construction code. Moving the calls inside the sub-test closure gives sharper failure attribution and full race coverage.♻️ Proposed refactor (shown for LegacyMatchesLegacyConstructor; apply the same pattern to PartitionedMatchesPartitionedConstructor)
cases := []struct { name string - dispatched []byte - legacy []byte + dispatch func() []byte + legacy func() []byte }{ - {"meta=nil data", sqsMsgDataKeyDispatch(nil, queue, 0, gen, msgID), - sqsMsgDataKey(queue, gen, msgID)}, + {"meta=nil data", + func() []byte { return sqsMsgDataKeyDispatch(nil, queue, 0, gen, msgID) }, + func() []byte { return sqsMsgDataKey(queue, gen, msgID) }, + }, // ... remaining cases follow the same pattern } for _, tc := range cases { tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() - require.Equal(t, tc.legacy, tc.dispatched, ...) + require.Equal(t, tc.legacy(), tc.dispatch(), ...) }) }Also applies to: 71-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adapter/sqs_keys_dispatch_test.go` around lines 15 - 63, The table builds dispatched and legacy byte slices eagerly which runs before sub-tests and t.Parallel(); move the calls to sqsMsgDataKeyDispatch, sqsMsgDataKey, sqsMsgVisKeyDispatch, sqsMsgVisKey, sqsMsgDedupKeyDispatch, sqsMsgDedupKey, sqsMsgGroupKeyDispatch, sqsMsgGroupKey, sqsMsgByAgeKeyDispatch, sqsMsgByAgeKey, and sqsMsgVisPrefixForQueueDispatch/sqsMsgVisPrefixForQueue inside the t.Run closure so each sub-test computes its own dispatched and legacy values (and any panics or race issues are attributed to the specific sub-test).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@adapter/sqs_keys_dispatch_test.go`:
- Around line 15-63: The table builds dispatched and legacy byte slices eagerly
which runs before sub-tests and t.Parallel(); move the calls to
sqsMsgDataKeyDispatch, sqsMsgDataKey, sqsMsgVisKeyDispatch, sqsMsgVisKey,
sqsMsgDedupKeyDispatch, sqsMsgDedupKey, sqsMsgGroupKeyDispatch, sqsMsgGroupKey,
sqsMsgByAgeKeyDispatch, sqsMsgByAgeKey, and
sqsMsgVisPrefixForQueueDispatch/sqsMsgVisPrefixForQueue inside the t.Run closure
so each sub-test computes its own dispatched and legacy values (and any panics
or race issues are attributed to the specific sub-test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2165b9b5-ba43-4632-80de-8c61f03e47eb
📒 Files selected for processing (2)
adapter/sqs_keys_dispatch.goadapter/sqs_keys_dispatch_test.go
PR #731 round 1 review findings: 1) Codex P2 — effectivePartitionCount must honor perQueue mode partitionFor (sqs_partitioning.go:76) forces every MessageGroupId to partition 0 when meta.FifoThroughputLimit == "perQueue", regardless of PartitionCount. effectivePartitionCount returned the literal PartitionCount in that mode, so ReceiveMessage's fanout would scan up to 31 guaranteed-empty partitions per poll for no correctness benefit — multiplying read / CPU work by the configured partition count. Fix: collapse to 1 when FifoThroughputLimit is the perQueue short-circuit. Mirrors partitionFor's routing decision. TestEffectivePartitionCount_PerQueueModeCollapsesToOne pins this with 4 sub-cases (perQueue + 4, perQueue + 32, perMessageGroupId + 4, empty + 4) so a future refactor that drops the branch is caught. 2) Claude finding 1 — expand legacy-match test coverage The PR description claimed nil and PartitionCount=0 sub-cases for all 6 helpers, but the actual table only had them for sqsMsgDataKeyDispatch. Expanded to cover all 6 helpers. Without this, a future edit that drops the `meta != nil` guard from one of the non-data helpers would compile and pass the existing test suite. TestSQSKeysDispatch_LegacyMatchesLegacyConstructor grew from 8 to 15 sub-cases. 3) Claude finding 2 — PartitionCount=2 boundary TestSQSKeysDispatch_PartitionedMatchesPartitionedConstructor used PartitionCount=4. An off-by-one in the dispatch threshold (>= 1 vs > 1) wouldn't be caught because 2 was never exercised. New TestSQSKeysDispatch_BoundaryAtPartitionCount2 pins the boundary cheaply. CodeRabbit nitpick on the eager-construction test pattern is acknowledged as "Low value" in their own classification — left as-is since the existing pattern is consistent with the project's other table-driven tests. go test -race ./adapter/... and golangci-lint clean.
|
@claude review Round 2 — addresses 3 round-1 findings: Codex P2 (real, fixed): Claude finding 1 (real, fixed): PR description claimed nil/PartitionCount=0 sub-cases for all 6 helpers, but the actual table only had them for Claude finding 2 (real, fixed): New CodeRabbit nitpick (eager construction in table rows, marked "Low value" in their own classification): left as-is to stay consistent with the project's other table-driven tests.
|
|
Claude finished @bootjp's task in 1m 53s —— View job Review of PR #731 — Round 2Todo
All three round-1 findings are correctly addressed. No new issues. Fix 1 — perQueue collapse in
|
1 participant
Summary
Foundation for PR 5b's Send/Receive partition fanout. Adds
sqsMsg*KeyDispatchwrappers that route to the legacy or partitioned constructor based onmeta.PartitionCount. Pure scaffold: helpers are added but no production call site uses them yet — those switch in PR 5b-2.Why a separate scaffold PR
PR 5b's full scope (Send/Receive fanout + receipt-handle v2 wiring + dormancy gate-lift + CreateQueue capability check) is ~1000 LOC across multiple files. The lessons-learned discipline from recent rounds (PR #723 missed the raftadmin stub because the audit was function-scoped, not interface-scoped) favours smaller diffs.
Splitting PR 5b into three:
PollSQSHTFIFOCapability. Small-but-critical.What's added
adapter/sqs_keys_dispatch.go(new file):sqsMsgDataKeyDispatch,sqsMsgVisKeyDispatch,sqsMsgDedupKeyDispatch,sqsMsgGroupKeyDispatch,sqsMsgByAgeKeyDispatch,sqsMsgVisPrefixForQueueDispatcheffectivePartitionCounthelper for fanout iterationEach helper takes
(meta, queueName, partition, gen, …)and routes to the legacy single-partition constructor whenmeta.PartitionCount <= 1(existing behaviour, byte-identical output) or to the partitioned constructor (#703's keyspace) when> 1.What does NOT change yet
adapter/sqs_messages.gostill call legacysqsMsg*Keyconstructors directly. PR 5b-2 swaps them.encodeReceiptHandleV2stays dormant.decodeClientReceiptHandlestill rejects v2 unconditionally.PartitionCount > 1withInvalidAttributeValue.Test plan
5 top-level tests (19 sub-cases):
TestSQSKeysDispatch_LegacyMatchesLegacyConstructor— 8 sub-cases pinning byte-identical output for nil meta,PartitionCount=0,PartitionCount=1across all 6 helpers.TestSQSKeysDispatch_PartitionedMatchesPartitionedConstructor— 6 sub-cases pinning byte-identical output forPartitionCount=4across all partitioned constructors.TestSQSKeysDispatch_LegacyAndPartitionedAreDistinct— keyspace isolation invariant.TestEffectivePartitionCount— iteration-count contract.TestSQSKeysDispatch_PartitionIgnoredOnLegacy— protects against stale-partition stranding on non-partitioned queues.go test -race ./adapter/...pass.golangci-lint ./adapter/...clean.Self-review (per CLAUDE.md)
TestSQSKeysDispatch_LegacyAndPartitionedAreDistinct. Stale-partition stranding pinned byTestSQSKeysDispatch_PartitionIgnoredOnLegacy. No issue.Summary by CodeRabbit
New Features
Tests