Conversation
1. Only need to split slice when post ckpt size is greater than the current size of the slice and greater than the slice upper bound. 2. For non snapshot read, there is no need to return the deleted keys from the data store.
WalkthroughThis pull request enhances snapshot read handling and batch-end key management in the data store service client closure, adds ownership tracking for batch start keys in the fill-store-slice request structure, and refines slice-splitting conditions and iterator rebinding logic in the transaction service's template map and local shard implementations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @store_handler/data_store_service_client_closure.cpp:
- Around line 1342-1352: The code sets fill_store_slice_req->kv_start_key_ to a
string_view of key.Data() and then moves key into AddDataItem, creating a
use-after-move; fix by creating a separate owner TxKey (e.g., owner_key via
catalog_factory->CreateTxKey) when i == items_size - 1 &&
scan_next_closure->ItemsSize() == 1000, assign
fill_store_slice_req->kv_start_key_ = std::string_view(owner_key.Data(),
owner_key.Size()) and store ownership in
fill_store_slice_req->kv_start_key_owner_ (move owner_key), then proceed to call
AddDataItem(std::move(key), ...); this mirrors the deleted-record path and
prevents the dangling string_view.
🧹 Nitpick comments (3)
tx_service/include/cc/cc_req_misc.h (1)
553-554: Document the ownership relationship betweenkv_start_key_owner_andkv_start_key_.The new field
kv_start_key_owner_appears to provide backing storage for thekv_start_key_view. Without documentation, the relationship between these fields and their intended usage pattern may not be clear to future maintainers. Consider adding a brief comment explaining thatkv_start_key_owner_holds the key data whilekv_start_key_provides a non-owning view into it.📝 Suggested documentation
public: // These variables only be used in DataStoreHandler const std::string *kv_table_name_{nullptr}; + // Owns the batch start key data; kv_start_key_ provides a view into this. TxKey kv_start_key_owner_; std::string_view kv_start_key_;tx_service/include/cc/template_cc_map.h (1)
5424-5429: Hardencheck_split_slice()againstUINT64_MAXsentinel and avoid repeatedPostCkptSize()calls.
The updated condition matches the PR objective, but the guard is currently debug-only (assert). In release builds, a sentinelUINT64_MAXwould make the predicate trivially true and could trigger pathological splitting.Proposed tweak
- assert(slice->PostCkptSize() != UINT64_MAX); - // Only need to split the slice when the post ckpt size of the slice - // is greater than the current size and greater than the slice upper - // bound. - return (slice->PostCkptSize() > StoreSlice::slice_upper_bound && - slice->PostCkptSize() > slice->Size()); + const uint64_t post_ckpt_size = slice->PostCkptSize(); + if (post_ckpt_size == UINT64_MAX) + { + return false; + } + // Split only when post-ckpt size exceeds both the slice upper bound + // and the current slice size. + return (post_ckpt_size > StoreSlice::slice_upper_bound && + post_ckpt_size > slice->Size());tx_service/src/cc/local_cc_shards.cpp (1)
5392-5414: Confirm intended behavior: “oversized but not growing” slices will no longer split.The new guard
slice_post_ckpt_size <= curr_slice->Size()matches the PR objective (split only when post-ckpt size exceeds both the upper bound and the current slice size). One implication is that a slice already aboveStoreSlice::slice_upper_boundbut not increasing won’t be split anymore—please confirm that’s desired for long-lived/legacy oversized slices.Proposed small clarity refactor
- if (slice_post_ckpt_size == UINT64_MAX || - slice_post_ckpt_size <= StoreSlice::slice_upper_bound || - slice_post_ckpt_size <= curr_slice->Size()) + const bool should_split = + slice_post_ckpt_size != UINT64_MAX && + slice_post_ckpt_size > StoreSlice::slice_upper_bound && + slice_post_ckpt_size > curr_slice->Size(); + if (!should_split) { // Case 1: There is no unpersisted data in the current slice, so no // need to split the slice. Only to migrate this data from the old // range to the new range. // Case 2: The current slice does not need to be split. assert(!status.paused_slice_); // Move to the next slice flush_record_it = slice_end_it; continue; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
store_handler/data_store_service_client_closure.cppstore_handler/eloq_data_store_service/eloqstoretx_service/include/cc/cc_req_misc.htx_service/include/cc/template_cc_map.htx_service/src/cc/local_cc_shards.cpp
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-21T06:46:53.700Z
Learnt from: lokax
Repo: eloqdata/tx_service PR: 149
File: src/remote/cc_stream_receiver.cpp:1066-1075
Timestamp: 2025-10-21T06:46:53.700Z
Learning: In src/remote/cc_stream_receiver.cpp, for ScanNextRequest handling, BucketIds() on RemoteScanNextBatch should never be empty—this is an expected invariant of the scan protocol.
Applied to files:
store_handler/data_store_service_client_closure.cpp
📚 Learning: 2025-11-11T07:10:40.346Z
Learnt from: lzxddz
Repo: eloqdata/tx_service PR: 199
File: include/cc/local_cc_shards.h:233-234
Timestamp: 2025-11-11T07:10:40.346Z
Learning: In the LocalCcShards class in include/cc/local_cc_shards.h, the EnqueueCcRequest methods use `shard_code & 0x3FF` followed by `% cc_shards_.size()` to distribute work across processor cores for load balancing. This is intentional and separate from partition ID calculation. The 0x3FF mask creates a consistent distribution range (0-1023) before modulo by actual core count.
Applied to files:
tx_service/src/cc/local_cc_shards.cpp
📚 Learning: 2025-10-09T03:56:58.811Z
Learnt from: thweetkomputer
Repo: eloqdata/tx_service PR: 150
File: include/cc/local_cc_shards.h:626-631
Timestamp: 2025-10-09T03:56:58.811Z
Learning: For the LocalCcShards class in include/cc/local_cc_shards.h: Writer locks (unique_lock) should continue using the original meta_data_mux_ (std::shared_mutex) rather than fast_meta_data_mux_ (FastMetaDataMutex) at this stage. Only reader locks may use the FastMetaDataMutex wrapper.
Applied to files:
tx_service/src/cc/local_cc_shards.cpp
📚 Learning: 2025-12-02T10:43:27.431Z
Learnt from: lokax
Repo: eloqdata/tx_service PR: 254
File: tx_service/src/cc/local_cc_shards.cpp:2949-3188
Timestamp: 2025-12-02T10:43:27.431Z
Learning: In tx_service/src/cc/local_cc_shards.cpp, whenever TryPinNodeGroupData is used, only call Sharder::Instance().UnpinNodeGroupData(node_group) if the recorded term is >= 0 (i.e., pin succeeded). Example: LocalCcShards::PostProcessFlushTaskEntries guards the unpin with `if (term >= 0)`.
Applied to files:
tx_service/src/cc/local_cc_shards.cpp
🧬 Code graph analysis (1)
tx_service/include/cc/cc_req_misc.h (1)
tx_service/include/range_record.h (1)
TxKey(256-262)
🔇 Additional comments (4)
tx_service/include/cc/template_cc_map.h (1)
11727-11735: Good fix: rebindabsl::btree_mapiterators aftererase()inMergePages().
Givenabsl::btree_mapiterator invalidation semantics, refindingmerged_pageafter erasingdiscarded_page_itavoids use-after-invalidation issues.store_handler/eloq_data_store_service/eloqstore (1)
1-1: Submodule update contains only test debugging changes, not the intended bug fixes mentioned in PR objectives.The submodule pointer update from
a94159ab298a...to67c86296b89a3...includes only 2 commits:
b8ecc272: "skip manifest checksum for test"67c8629: "do not append manifest" (disables manifest appending viafalse &&condition in write_task.cpp)These changes focus on manifest handling and appear to be debugging/test modifications, not the slice-splitting logic fixes or non-snapshot read bug fixes stated in the PR objectives. This mismatch between the submodule content and PR goals should be clarified—either the PR description needs updating or the submodule pointer should reference commits containing the intended fixes.
store_handler/data_store_service_client_closure.cpp (2)
1279-1293: LGTM — deferred record creation and snapshot timestamp extraction.The changes correctly defer
TxRecordcreation and read the snapshot timestamp to gate deleted-record handling. This aligns with the PR objective of not returning deleted keys for non-snapshot reads.
1310-1333: LGTM — correctly skips visible deleted records and preserves batch-key ownership.The logic properly distinguishes between:
- Non-snapshot reads: always skip deleted records
- Snapshot reads: skip deleted records only if they're visible (committed before snapshot), keeping those needing archive lookup
The batch-end key ownership is correctly preserved via
kv_start_key_owner_.
| txservice::TxKey key = | ||
| catalog_factory->CreateTxKey(key_str.data(), key_str.size()); | ||
| if (i == items_size - 1 && scan_next_closure->ItemsSize() == 1000) | ||
| { | ||
| // Record the last key of the current batch as the start key of the | ||
| // next batch. | ||
| fill_store_slice_req->kv_start_key_ = | ||
| std::string_view(key.Data(), key.Size()); | ||
| } | ||
|
|
||
| fill_store_slice_req->AddDataItem( | ||
| std::move(key), std::move(record), ts, is_deleted); |
There was a problem hiding this comment.
Use-after-move: kv_start_key_ becomes a dangling reference after key is moved.
When this is the last item in a 1000-item batch, kv_start_key_ is set to point to key.Data(), but then key is moved into AddDataItem. The string_view will reference invalid memory when used in ScanNext (line 1362).
The deleted-record path (lines 1326-1330) correctly preserves ownership via kv_start_key_owner_. The same pattern should be applied here.
🔧 Proposed fix
txservice::TxKey key =
catalog_factory->CreateTxKey(key_str.data(), key_str.size());
if (i == items_size - 1 && scan_next_closure->ItemsSize() == 1000)
{
// Record the last key of the current batch as the start key of the
// next batch.
fill_store_slice_req->kv_start_key_ =
std::string_view(key.Data(), key.Size());
+ fill_store_slice_req->kv_start_key_owner_ = key.Clone();
}
fill_store_slice_req->AddDataItem(
std::move(key), std::move(record), ts, is_deleted);Note: This assumes TxKey has a Clone() method or copy constructor. If not, you may need to create a separate key for ownership:
if (i == items_size - 1 && scan_next_closure->ItemsSize() == 1000)
{
txservice::TxKey owner_key = catalog_factory->CreateTxKey(key_str.data(), key_str.size());
fill_store_slice_req->kv_start_key_ =
std::string_view(owner_key.Data(), owner_key.Size());
fill_store_slice_req->kv_start_key_owner_ = std::move(owner_key);
}🤖 Prompt for AI Agents
In @store_handler/data_store_service_client_closure.cpp around lines 1342 -
1352, The code sets fill_store_slice_req->kv_start_key_ to a string_view of
key.Data() and then moves key into AddDataItem, creating a use-after-move; fix
by creating a separate owner TxKey (e.g., owner_key via
catalog_factory->CreateTxKey) when i == items_size - 1 &&
scan_next_closure->ItemsSize() == 1000, assign
fill_store_slice_req->kv_start_key_ = std::string_view(owner_key.Data(),
owner_key.Size()) and store ownership in
fill_store_slice_req->kv_start_key_owner_ (move owner_key), then proceed to call
AddDataItem(std::move(key), ...); this mirrors the deleted-record path and
prevents the dangling string_view.
3 participants
Fix slice split
Summary by CodeRabbit
Release Notes
Bug Fixes
Performance Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.