Fix DSS drain gates - #521
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughDataStoreService tracks read and scan submission windows per thread and shard, uses RAII guards across read paths, and waits for those windows to drain before datastore shutdown. Related atomic operations now use sequential consistency, and the eloqstore submodule pointer changes. ChangesRead submit window quiescing
Eloqstore revision
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ReadScanHandlers
participant DataStoreService
participant Datastore
ReadScanHandlers->>DataStoreService: EnterReadSubmitWindow(shard_id)
DataStoreService->>Datastore: Submit read or scan request
DataStoreService-->>ReadScanHandlers: ReadSubmitGuard
ReadScanHandlers->>DataStoreService: Guard destruction
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
27cc31e to
f6082b3
Compare
f6082b3 to
9738f70
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
store_handler/eloq_data_store_service/data_store_service.h (2)
716-741: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ReadSubmitSlot::depthneeds a trailing underscore.Per the Google C++ style guidelines used in this repo, member variables must use snake_case with a trailing underscore (e.g.
my_name_).depthshould bedepth_.As per coding guidelines, "Member variables should use snake_case with trailing underscore (e.g., `my_name_`)".🔧 Proposed fix
struct alignas(64) ReadSubmitSlot { - std::atomic<uint32_t> depth{0}; + std::atomic<uint32_t> depth_{0}; };(update the three call sites in data_store_service.cpp accordingly:
slot.depth→slot.depth_)🤖 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 `@store_handler/eloq_data_store_service/data_store_service.h` around lines 716 - 741, Rename the ReadSubmitSlot member depth to depth_ in data_store_service.h, then update all three corresponding slot.depth references in data_store_service.cpp to slot.depth_.Source: Coding guidelines
198-200: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffStatic sizing multiplies to a large fixed memory footprint.
data_shards_is nowstd::array<DataShard, kMaxShardCount>(1000), and eachDataShardembedsstd::array<ReadSubmitSlot, kReadSubmitSlotCount>(1000) whereReadSubmitSlotisalignas(64)(i.e. 64 bytes per slot). That alone is1000 × 1000 × 64 B ≈ 62 MiBof always-resident memory perDataStoreServiceinstance, independent of how many shards/threads are actually in use on a given node. If a node only owns a handful of shards, nearly all of that memory is wasted.Consider sizing
read_submit_slots_/kReadSubmitSlotCountto the actual expected worker/thread concurrency (e.g. bthread worker pool size) rather than a flat 1000, or lazily allocate the slots array only for shards this node actually owns.Also applies to: 822-830
🤖 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 `@store_handler/eloq_data_store_service/data_store_service.h` around lines 198 - 200, Reduce the fixed memory footprint in DataStoreService by removing the flat 1000-slot sizing for each DataShard’s read_submit_slots_ and sizing it to actual worker/thread concurrency, or allocating it only for owned shards. Update kReadSubmitSlotCount and the read_submit_slots_ storage/initialization together, while preserving support for the configured concurrency and existing read submission behavior.
🤖 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.
Nitpick comments:
In `@store_handler/eloq_data_store_service/data_store_service.h`:
- Around line 716-741: Rename the ReadSubmitSlot member depth to depth_ in
data_store_service.h, then update all three corresponding slot.depth references
in data_store_service.cpp to slot.depth_.
- Around line 198-200: Reduce the fixed memory footprint in DataStoreService by
removing the flat 1000-slot sizing for each DataShard’s read_submit_slots_ and
sizing it to actual worker/thread concurrency, or allocating it only for owned
shards. Update kReadSubmitSlotCount and the read_submit_slots_
storage/initialization together, while preserving support for the configured
concurrency and existing read submission behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7d0a021-c6d2-4856-bbf7-5d4990f3f2fc
📒 Files selected for processing (3)
store_handler/eloq_data_store_service/data_store_service.cppstore_handler/eloq_data_store_service/data_store_service.hstore_handler/eloq_data_store_service/eloqstore
|
I have performed a benchmark on an 8-core ARM VM. No degradation is found. |
9738f70 to
631a8b2
Compare
2 participants
Summary
This PR closes two DataStoreService shutdown/drain races:
ReadWrite -> ReadOnlywhile the drainer observes zero in-flight writes.ReadOnly -> Closedbegins draining, which is required for EloqStore becauseStop()must not race with a laterExecAsyn()submission.It also advances the nested
eloqstoresubmodule to the latestmaincommit:acd3cb3 fix: complete requests racing Stop() instead of crashing or hanging (#483)Details
Write drain gate
The write path already increments
ongoing_write_requests_before checking shard status. However, the previous ordering used release/acquire operations, which does not impose one global order across the writer's count increment/status load and the drainer's status CAS/count load.This PR changes the write gate operations to
seq_cst:ReadWrite -> ReadOnlyCASWith one sequentially consistent order, the unsafe outcome where a writer still observes
ReadWritewhile the drainer already observes count0is prevented.Read/scan submit drain gate
Read and scan paths now enter a per-thread submit window before loading shard status.
ReadOnly -> Closedfirst switches the shard status toStarting, then waits for all submit windows to drain before calling backendShutdown().This blocks the problematic interleaving:
The submit window is intentionally scoped only around the status check and backend submission. It is not a full async request lifetime counter.
That is deliberate for EloqStore: the bug is submission-after-stop, not waiting for every accepted read callback before close. Extending the counter to
SetFinish()would add a cross-thread atomic decrement to the hot read path.For RocksDB/RocksDBCloud, accepted reads/scans are already safe because their async behavior is implemented by DataStoreService's
query_worker_pool_.Shutdown()joins the worker pool, and workers drain queued work before exiting, so the DB is closed/deleted only after accepted worker tasks complete.Validation
cmake --build bld-eloqstore-local --target eloqkv -j2git diff --check -- store_handler/eloq_data_store_service/data_store_service.cpp store_handler/eloq_data_store_service/data_store_service.hSummary by CodeRabbit