Unify entity-store full scans behind a bounded paged primitive#873
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthrough
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/entity/scan.go (1)
60-75: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPin the etcd revision across pages.
scanPagedissues each page at the current revision, so concurrent writes can make a full scan miss or duplicate keys between requests. Captureresp.Header.Revisionon the first page and passclientv3.WithRev(...)on the rest so these list operations keep snapshot semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/entity/scan.go` around lines 60 - 75, The scanPaged loop currently reissues each etcd page without fixing the revision, which can make results inconsistent across concurrent writes. In scan.go, update scanPaged to capture resp.Header.Revision from the first successful client.Get call and reuse it for subsequent requests by adding clientv3.WithRev(...) to getOpts after the first page. Keep the existing cursor advancement logic, but ensure all later pages are fetched from the same snapshot revision so the full scan remains stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/entity/scan.go`:
- Around line 60-75: The scanPaged loop currently reissues each etcd page
without fixing the revision, which can make results inconsistent across
concurrent writes. In scan.go, update scanPaged to capture resp.Header.Revision
from the first successful client.Get call and reuse it for subsequent requests
by adding clientv3.WithRev(...) to getOpts after the first page. Keep the
existing cursor advancement logic, but ensure all later pages are fetched from
the same snapshot revision so the full scan remains stable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1f835e2-27e0-4376-97ab-a50c73218c9f
📒 Files selected for processing (7)
components/coordinate/coordinate.gopkg/entity/migrate.gopkg/entity/reindex.gopkg/entity/scan.gopkg/entity/scan_test.gopkg/entity/shortid_migrate.gopkg/entity/store.go
#872 added listEntitiesPaged to bound two startup scans, but the principled home is one primitive every full-keyspace read shares. Rename it scanPaged, give it keys-only and page-size options plus an EtcdStore method, and route the three reads #872 missed through it: the short-id unique-key set, ListAllEntityIDs, and reindex's stale-cleanup scan. No startup scan open-codes Get(WithPrefix()) now. Also collapse the reindex timeout nesting from #871. checkAndReindex's inner 5-min budget always clamped to whatever was left of the 2-min startup ceiling, so it was dead. Drop it and let reindex share the single 2-min bound that keeps the coordinator off the edge listener's back; an unfinished reindex retries on the next boot.
phinze
force-pushed
the
phinze/mir-1272-unify-entity-store-full-scans-behind-a-bounded-paged
branch
from
411262c to
63ed275
Compare
phinze
deleted the
phinze/mir-1272-unify-entity-store-full-scans-behind-a-bounded-paged
branch
2 participants
Follow-on cleanup to #871 and #872, which fixed a coordinator parking in
starting entity migrationfor ~7.5h after a restart. The startup maintenance phase reads the whole entity keyspace before the edge listener binds, and on a bloated etcd that unbounded read stalled indefinitely. #871 added a 2-minute timeout (the net) and #872 paginated two of the reads (the fix).The catch is that #872's paging helper only covered two of the five full-keyspace reads in the startup path; the other three (the short-id unique-key set,
ListAllEntityIDs, and reindex's stale-cleanup scan) still open-coded their ownGet(WithPrefix()). So instead of leaving bounding as something each call site has to remember, we promote the helper to one primitive every read shares.listEntitiesPagedbecomesscanPagedwith keys-only and page-size options plus a thinEtcdStoremethod, and all five scans now route through a single paging loop.While in here, we also untangled the timeout nesting. Reindex had its own inner 5-minute deadline, but it was derived from #871's outer 2-minute ceiling, so it could only ever clamp down to whatever was left of the 2 minutes. It never granted reindex anything. We dropped it and let reindex share the single 2-minute bound, which is the real guarantee anyway; an unfinished reindex is hash-gated to retry on the next boot.
Closes MIR-1272