adapter: split dynamodb.go into cohesive files (no behavior change)#957
adapter: split dynamodb.go into cohesive files (no behavior change)#957bootjp wants to merge 4 commits into
Conversation
… files (no behavior change) Pure code movement within package adapter: schema+table management, storage key prefix/segment encoding, attribute-value compare/clone/ key-encode helpers, and DynamoDB error mapping/transient classifier move out of dynamodb.go into dedicated dynamodb_schema.go / dynamodb_keys.go / dynamodb_attrvalue.go / dynamodb_errors.go. No declarations changed (byte-identical, gofmt; imports recomputed per file via goimports).
…hesive files (no behavior change) Pure code movement within package adapter: GetItem read path, Put/Update/Delete write paths + one-phase dedup, Query/Scan + read iterators + key-condition parsing, BatchWrite/TransactWrite/TransactGet, and the lease pre-pass helpers (leaseCheckScan/leaseCheckQuery/ leaseCheckTransactGetItems, multiShardReadLeasePlan, queryLeaseKey, leaseReadGroupKeys) move into dynamodb_item_read.go / dynamodb_item_write.go / dynamodb_query_scan.go / dynamodb_transact.go / dynamodb_lease.go. No declarations changed.
…iles (no behavior change) Pure code movement within package adapter: update-expression parsing, condition-expression evaluation, document-path traversal move into dynamodb_expression.go; legacy table key migration into dynamodb_migration.go; item/table lock striping + txn read-ts helpers into dynamodb_locks.go. No declarations changed.
…hange) dynamodb.go now holds only the server core/registration: DynamoDBServer struct + options + constructor, Run/Stop, HTTP handler/dispatch/health, body-size readers, and the request-metrics recorder. Imports trimmed to what the core uses. No declarations changed.
|
Warning Review limit reached
More reviews will be available in 33 minutes and 46 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✨ 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. Comment |
|
@claude review |
|
Claude finished @bootjp's task in 2m 35s —— View job PR Review: adapter: split dynamodb.go into cohesive filesTasks
SummaryThis is a well-executed behavior-preserving refactor. The split is logically organized, import hygiene is correct, and no duplicate declarations were introduced. CI is still running (Test + Jepsen in progress; Docker Image CI and Reviewdog both passed). FindingsMinor:
|
| Check | Result |
|---|---|
encoding/json accidentally used instead of json "github.com/goccy/go-json" |
None found — all files that decode/encode JSON use the correct goccy alias |
Duplicate func declarations across split files |
None found |
Duplicate type declarations across split files |
None found |
package adapter header in all new files |
Correct |
Constants in dynamodb.go still reference kv.*Prefix correctly |
Correct — dynamoTableMetaPrefix, dynamoItemPrefix, etc. still alias kv.* |
snapshotTS (used in dynamodb_keys.go) defined in package |
Correct — lives in adapter/ts.go:17 |
Imports: dynamoErrIsTransient comment preserved with the function |
Correct — the detailed "lease pre-pass" comment moved intact to dynamodb_errors.go |
| CI: Docker Image CI | ✅ passed |
| CI: Reviewdog (lint) | ✅ passed |
| CI: Test suite | ⏳ in progress |
| CI: Jepsen | ⏳ in progress |
Code organization quality
The concern-per-file split is clean and sensible:
dynamodb_errors.go— error types + response helpers (a natural unit)dynamodb_locks.go— lock striping + read-ts pin (tight coupling:lockItemUpdate,nextTxnReadTS,pinReadTSall affect write-path isolation)dynamodb_lease.go— lease pre-pass guards (correctly co-located with their lease timeout constant, which stayed indynamodb.go)dynamodb_migration.go— legacy key migration (self-contained lifecycle)dynamodb_expression.go— expression parsing/evaluation (largest standalone concern)
No issues with the split itself. Approachable as a reader now.
Verdict
Looks good. The two items above (missing table row and line-count nit in the description) are documentation-only. The code changes are purely mechanical file splits with no logic alterations. Once the Test and Jepsen CI runs complete green, this is ready to merge.
![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 comprehensive DynamoDB-compatible adapter layer for the elastickv store, implementing attribute value handling, expressions, item reads/writes, transactional operations, GSI support, and legacy table migrations. A critical compilation and synchronization issue was identified in the GSI read worker pool initialization where a non-existent Go method is called on sync.WaitGroup.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for range workerCount { | ||
| wg.Go(func() { | ||
| d.gsiReadWorker(ctx, readTS, filter, jobs, results, cancel) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The sync.WaitGroup type from the standard library does not have a Go method. Using wg.Go will cause a compilation failure. Additionally, because wg.Add is never called, wg.Wait() would return immediately, leading to a race condition where the results channel is closed while workers are still running.
Update this to use the standard wg.Add(1) and go func() pattern with defer wg.Done() to ensure correct synchronization.
for range workerCount {
wg.Add(1)
go func() {
defer wg.Done()
d.gsiReadWorker(ctx, readTS, filter, jobs, results, cancel)
}()
}1 participant
Summary
Behavior-preserving refactor: split the 9,328-line
adapter/dynamodb.gointo cohesive same-package (package adapter) files. This is pure code movement — types/functions/methods/consts are relocated with no signature changes, no logic changes, no renames, and no comment rewrites (comments move with their code). The compiled package is semantically identical.dynamodb.goshrinks from 9,328 → 553 lines and now holds only the server core/registration (struct, options, constructor,Run/Stop, HTTP handler/dispatch/health, body-size readers, request-metrics recorder). 526 top-level declarations were relocated into 12 new files; 49 stayed indynamodb.go.File map (old region → new file)
dynamodb.go(kept)DynamoDBServerstruct/options/NewDynamoDBServer,Run/Stop,proxyToLeader,handle/dispatchByTarget/dispatchOrWriteUnsupported, health-check handlers,maxDynamoBodyReader,countingReadCloser/dynamoResponseRecorder, request-metrics recorder/observers, target consts +dynamoOperationTargetsdynamodb_schema.gocreateTable*,dynamoKeySchema,dynamoGlobalSecondaryIndex,dynamoTableSchema),*dynamoTableSchemakey-derivation methods (primary/GSI/legacy key builders,primaryKeyValues),describeTableShape,loadTableSchema*/loadTableGenerationAtdynamodb_item_read.gogetItemInput,getItem,parseGetItemInput,resolveGetItemTarget,readItemAtKeyAt,readLogicalItemAtdynamodb_item_write.goretryItemWriteWithGeneration*,prepare{Put,Update,Delete}ItemWrite,buildItemWriteRequestWithSource, return-attribute helpersdynamodb_query_scan.gotableReadIterator/gsiReadIterator/keyRangeKVIterator),readPageState, read-bounds resolution, projection, GSI parallel read workers, key-condition expression parsing/matching, scan/query exclusive-start-key & projection validatorsdynamodb_transact.gocleanupCommittedKeysdynamodb_lease.goleaseReadKeyless,leaseCheckScan,leaseCheckQuery,leaseCheckTransactGetItems,multiShardReadLeasePlan,queryLeaseKey/queryLeasePrefix/gsiQueryLeasePlan,resolveTransactGetItemKeys,leaseReadGroupKeys,transactGetItemKey/transactGetItemKeyFromSchemadynamodb_expression.godynamodb_attrvalue.goattributeValueAsKey*, numeric/ordered-int encoders, segment escaping), equality/sort comparisons, clone helpers,reverseItemsdynamodb_errors.godynamoAPIError,writeDynamoError*,writeDynamoJSON,dynamoErrIsTransient,newDynamoAPIErrordynamodb_keys.godynamoTableMetaKey/dynamoItemKey/dynamoGSIKey(+ legacy variants),encode/decodeDynamoSegment,scanAllByPrefix*,nextScanCursor,minIntdynamodb_migration.goensureLegacyTableMigration*,migrateLegacy*,finalizeLegacyTableMigrationdynamodb_locks.golockItemUpdate/lockTableOperations,stripeIndex,dynamoItemUpdateLockKey,nextTxnReadTS/pinReadTS,globalLastCommitTSProvider(Pre-existing split files
dynamodb_types.go,dynamodb_storage_codec.go,dynamodb_admin.go,dynamodb_admin_items.gowere not touched. Test files were not split.)No behavior change
This PR changes only the physical location of declarations within
package adapter. No declaration body, signature, name, or comment was altered. Per-file imports were recomputed bygoimports(verified: noencoding/jsonsubstituted for the aliasedjson "github.com/goccy/go-json"; no other import-path changes).Verification evidence
go build ./...→ exit 0go vet ./adapter/→ exit 0gofmt -l adapter/→ empty (clean)go test -race -count=1 -run 'TestDynamoDB' -timeout 900s ./adapter/→ok ... 23.3s(cache defeated with-count=1)golangci-lint --config=.golangci.yaml run ./adapter/→ 0 issuesgo doc -all ./adapter— identical between trueorigin/mainbaseline (captured viagit stash) and this branch (byte-for-byte diff empty). Confirms the full exported + unexported API surface is unchanged.go/printer-based dumper printed every top-level declaration (sorted by name) from the originaldynamodb.goand from the union of the new files;diffis empty → all 575 declarations byte-identical after gofmt normalization.git diff --stat:dynamodb.go-8,788 lines; new files +8,941 (delta = the 12package+import headers and blank separators).Five-lens self-review
All five are n/a — pure move (no logic touched). Evidence:
go doc -allidentical, per-declaration byte-identical, full DynamoDB test suite green under-race.go doc -all+ per-decl diff confirm identical bodies.dynamodb_locks.go), lease pre-pass (dynamodb_lease.go), retry/backoff, and GSI worker code moved verbatim.go test -racegreen.nextTxnReadTS/pinReadTS, lease checks, generation-fence verification) moved unchanged.*_test.gofiles (package-internal) remain valid and pass. A pure relocation adds no new behavior to test.