feat: partition-level reopen for buffered commands on standby - #484
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR threads a new boolean reopen flag from clients through RPCs into the data store and the underlying KV layer, adds reopen-aware local request handling and config, and propagates reopen through TX service FetchRecord request creation and buffered-command follow-ups. ChangesPartition Reopen API and Orchestration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tx_service/include/cc/object_cc_map.h (1)
2650-2775:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
normal_obj_sz_when BackFill overwrites an already-loaded entry.After changing this guard to
Unknown || older-version, the block now runs for CCEs that were already counted innormal_obj_sz_. The tail still unconditionally increments onNormaland never decrements when a previously normal object is replaced byDeleted, so reopen/backfill will drift the object count over time.💡 Track the previous payload state before overwriting it
- if (cce->PayloadStatus() == RecordStatus::Unknown || + RecordStatus prev_status = cce->PayloadStatus(); + if (prev_status == RecordStatus::Unknown || cce->CommitTs() < commit_ts) { cce->SetCommitTsPayloadStatus(commit_ts, status); cce->SetCkptTs(commit_ts); DLOG(INFO) << "BackFill key: " << cce->KeyString() @@ - if (cce->PayloadStatus() == RecordStatus::Normal) + if (cce->PayloadStatus() == RecordStatus::Normal) { - TemplateCcMap<KeyT, ValueT, false, false>::normal_obj_sz_++; + if (prev_status != RecordStatus::Normal) + { + TemplateCcMap<KeyT, ValueT, false, false>::normal_obj_sz_++; + } if (cce->payload_.cur_payload_ && cce->payload_.cur_payload_->HasTTL() && ccp->smallest_ttl_ > cce->payload_.cur_payload_->GetTTL()) { ccp->smallest_ttl_ = cce->payload_.cur_payload_->GetTTL(); } } else { + if (prev_status == RecordStatus::Normal) + { + TemplateCcMap<KeyT, ValueT, false, false>::normal_obj_sz_--; + } assert(cce->PayloadStatus() == RecordStatus::Deleted); ccp->smallest_ttl_ = 0; }🤖 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 `@tx_service/include/cc/object_cc_map.h` around lines 2650 - 2775, The BackFill block (where cce->SetCommitTsPayloadStatus is called) can double-count objects because TemplateCcMap::normal_obj_sz_ is incremented whenever cce->PayloadStatus() == RecordStatus::Normal after the overwrite, but it never decrements if the prior payload was Normal and becomes Deleted; before mutating the CCE (in the BackFill path in object_cc_map code handling cce and payload_), capture the previous payload status (e.g., prev_status = cce->PayloadStatus() or inspect cce->payload_.cur_payload_), then after applying DeserializeCurrentPayload/SetCommitTsPayloadStatus compute the transition (prev_status -> new_status) and adjust TemplateCcMap<KeyT,ValueT,false,false>::normal_obj_sz_ accordingly (increment only on Deleted->Normal, decrement on Normal->Deleted, no-op otherwise), ensuring to use the same cce, commit_ts, and commit_status branches so counts remain consistent.
🧹 Nitpick comments (2)
store_handler/eloq_data_store_service/data_store.h (1)
118-129: ⚡ Quick winDocument callback-vs-return contract for
ReopenPartitionno-op path
store_handler/eloq_data_store_service/data_store.hdefault callscallback()and returnsfalse.- The current caller (
tx_service/src/cc/cc_shard.cpp::CcShard::RequestPartitionReopen) ignores the return value and relies on the callback;EloqStoreDataStore::OnReopenPartitionmoves and invokes the callback once.- The differing behavior from
tx_service/include/store/data_store_handler.hdefault (returnsErrorwithout invoking the callback) should be captured in the interface contract so callers implement only one completion mechanism.🤖 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.h` around lines 118 - 129, The default in DataStore::ReopenPartition currently calls callback() and returns false, which conflicts with the other interface (store/data_store_handler.h) and caller patterns (CcShard::RequestPartitionReopen expects the callback; EloqStoreDataStore::OnReopenPartition moves+invokes the callback once). Update the interface contract: document in the ReopenPartition comment that implementations must choose one completion mechanism—either (A) handle synchronously and return false without invoking the callback, or (B) handle asynchronously, return true to indicate they will invoke the provided callback exactly once—and adjust the default DataStore::ReopenPartition implementation to NOT call callback and return false to match the error-return default in data_store_handler.h; ensure callers (CcShard::RequestPartitionReopen, EloqStoreDataStore::OnReopenPartition) follow the documented contract.store_handler/eloq_data_store_service/eloq_store_data_store.cpp (1)
1082-1087: 💤 Low valueUnused parameter
is_hash_partitioned.This parameter is declared but never used. Consider adding
[[maybe_unused]]or casting to void to suppress warnings and document intent.🤖 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/eloq_store_data_store.cpp` around lines 1082 - 1087, The parameter is_hash_partitioned in the EloqStoreDataStore::ReopenPartition signature is unused and triggers warnings; to fix, mark it explicitly unused by either adding the [[maybe_unused]] attribute to the parameter in the function declaration/definition or add a single-line cast (void)is_hash_partitioned; at the start of the EloqStoreDataStore::ReopenPartition body to document intent and suppress warnings while leaving behavior unchanged.
🤖 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.
Inline comments:
In `@store_handler/eloq_data_store_service/eloq_store_data_store.cpp`:
- Around line 1122-1133: The local callback is moved into op via
op->Reset(std::move(callback)) so the function-scoped callback is empty when
ExecAsyn fails; ensure you preserve and invoke the original callback on async
submission failure by saving a copy before the move (e.g., auto saved_cb =
callback) or by changing the call order so you call op->Reset(...) with a copy,
then on ExecAsyn failure call saved_cb() (or invoke the saved std::function)
before op->Clear(); touch the ExecAsyn call site, op->Reset, and the failure
branch that currently checks callback to guarantee the callback is invoked.
In `@tx_service/include/cc/cc_shard.h`:
- Around line 1295-1305: The map pending_partition_reopens_ uses std::string
keys which loses full TableName identity and can cause cross-table aliasing;
change its key type to the project's TableName type (the same key used by
CcShard for other table-scoped state) so reopen state is isolated per TableName,
update the declaration of pending_partition_reopens_ and any places that
insert/lookup to use TableName instead of std::string, and ensure
PartitionReopenState and its members (in_flight, cces) remain unchanged while
compilation adjustments are made where the map is referenced.
In `@tx_service/src/cc/cc_shard.cpp`:
- Around line 2090-2114: The code erases the pending_partition_reopens_ entry
before calling FetchRecord, so if FetchRecord returns Retry the reopen is lost
and some CCE pins remain; fix by deferring removal of the
pending_partition_reopens_ entry until after processing cces (do not call
it->second.erase(part_it) / ccs.pending_partition_reopens_.erase(it) up-front),
iterate the cces and call FetchRecord for each, always call
cce_ptr->GetKeyGapLockAndExtraData()->ReleasePin() to balance the earlier pin
regardless of FetchRecord result, and if any FetchRecord returns Retry
re-enqueue or keep the pending_partition_reopens_ entry (only remove it when all
FetchRecord calls complete without Retry) so buffered commands are retried
correctly.
---
Outside diff comments:
In `@tx_service/include/cc/object_cc_map.h`:
- Around line 2650-2775: The BackFill block (where cce->SetCommitTsPayloadStatus
is called) can double-count objects because TemplateCcMap::normal_obj_sz_ is
incremented whenever cce->PayloadStatus() == RecordStatus::Normal after the
overwrite, but it never decrements if the prior payload was Normal and becomes
Deleted; before mutating the CCE (in the BackFill path in object_cc_map code
handling cce and payload_), capture the previous payload status (e.g.,
prev_status = cce->PayloadStatus() or inspect cce->payload_.cur_payload_), then
after applying DeserializeCurrentPayload/SetCommitTsPayloadStatus compute the
transition (prev_status -> new_status) and adjust
TemplateCcMap<KeyT,ValueT,false,false>::normal_obj_sz_ accordingly (increment
only on Deleted->Normal, decrement on Normal->Deleted, no-op otherwise),
ensuring to use the same cce, commit_ts, and commit_status branches so counts
remain consistent.
---
Nitpick comments:
In `@store_handler/eloq_data_store_service/data_store.h`:
- Around line 118-129: The default in DataStore::ReopenPartition currently calls
callback() and returns false, which conflicts with the other interface
(store/data_store_handler.h) and caller patterns
(CcShard::RequestPartitionReopen expects the callback;
EloqStoreDataStore::OnReopenPartition moves+invokes the callback once). Update
the interface contract: document in the ReopenPartition comment that
implementations must choose one completion mechanism—either (A) handle
synchronously and return false without invoking the callback, or (B) handle
asynchronously, return true to indicate they will invoke the provided callback
exactly once—and adjust the default DataStore::ReopenPartition implementation to
NOT call callback and return false to match the error-return default in
data_store_handler.h; ensure callers (CcShard::RequestPartitionReopen,
EloqStoreDataStore::OnReopenPartition) follow the documented contract.
In `@store_handler/eloq_data_store_service/eloq_store_data_store.cpp`:
- Around line 1082-1087: The parameter is_hash_partitioned in the
EloqStoreDataStore::ReopenPartition signature is unused and triggers warnings;
to fix, mark it explicitly unused by either adding the [[maybe_unused]]
attribute to the parameter in the function declaration/definition or add a
single-line cast (void)is_hash_partitioned; at the start of the
EloqStoreDataStore::ReopenPartition body to document intent and suppress
warnings while leaving behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f95c744a-e1b9-4241-b6a7-18c312a47b63
📒 Files selected for processing (13)
store_handler/data_store_service_client.cppstore_handler/data_store_service_client.hstore_handler/eloq_data_store_service/data_store.hstore_handler/eloq_data_store_service/data_store_service.cppstore_handler/eloq_data_store_service/data_store_service.hstore_handler/eloq_data_store_service/eloq_store_data_store.cppstore_handler/eloq_data_store_service/eloq_store_data_store.hstore_handler/eloq_data_store_service/eloqstoretx_service/include/cc/cc_shard.htx_service/include/cc/object_cc_map.htx_service/include/store/data_store_handler.htx_service/src/cc/cc_req_misc.cpptx_service/src/cc/cc_shard.cpp
thweetkomputer
force-pushed
the
feat-committs-gap-trigger-reopen-zc
branch
2 times, most recently
from
June 2, 2026 02:04
e16f770 to
4059f79
Compare
thweetkomputer
force-pushed
the
feat-committs-gap-trigger-reopen-zc
branch
from
June 3, 2026 04:32
6d8fc96 to
19fa12b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
store_handler/data_store_service_client.cpp (1)
689-699:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRoute reopen through the owning DSS shard.
shard_idis computed on Lines 686-687, but this path still always calls the localdata_store_service_. UnlikeRead,DeleteRange,ScanNext, andBatchWriteRecordsin this file, there is noIsLocalShard(shard_id)/ RPC split here, so a reopen for a remote-owned partition will be sent to the wrong node and never reach the shard that can actually reopen it.🤖 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/data_store_service_client.cpp` around lines 689 - 699, The ReopenPartition call is always sent to the local data_store_service_ even when shard_id is owned remotely; add the same local/remote split used by Read/BatchWriteRecords/DeleteRange/ScanNext: check IsLocalShard(shard_id) and if true call data_store_service_->ReopenPartition(..., std::move(callback)); otherwise route the request to the shard owner via the RPC path (e.g. invoke the existing RPC client used by other methods or a ReopenPartition RPC that forwards the same arguments and bridges the callback), ensuring the shard ownership resolution (shard_id → owner node) and callback forwarding match the patterns in Read/BatchWriteRecords to deliver results back to the caller.store_handler/eloq_data_store_service/eloq_store_data_store.cpp (1)
66-70:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDon't read
opafterClear().
ReopenOperationData::Clear()on Lines 66-70 nullscallback_and frees the pooled object. The failure branch then readsop->callback_afterop->Clear(), so the callback is lost and Line 1130 becomes a use-after-free. That can leave the reopen completion path stuck whenExecAsyn()submission fails.🐛 Minimal fix
if (!eloq_store_service_->ExecAsyn( &reopen_req, user_data, OnReopenPartition)) { LOG(ERROR) << "ReopenPartition: failed to submit reopen for table " << table_name << " partition " << partition_id; - op->Clear(); - // Note: callback was moved into op by Reset() above. - // Access it through op->callback_ to avoid using the - // moved-from local. - if (op->callback_) + auto cb = std::move(op->callback_); + op->Clear(); + if (cb) { - op->callback_(); - op->callback_ = nullptr; + cb(); } return false; }#!/bin/bash sed -n '66,70p' store_handler/eloq_data_store_service/eloq_store_data_store.cpp printf '\n---\n' sed -n '1123,1134p' store_handler/eloq_data_store_service/eloq_store_data_store.cppAlso applies to: 1123-1134
🤖 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/eloq_store_data_store.cpp` around lines 66 - 70, ReopenOperationData::Clear() currently nulls callback_ and calls Free(), so callers that inspect op->callback_ after op->Clear() get a use-after-free; fix the caller that handles ExecAsyn() failure by capturing the callback before clearing the op (e.g., store op->callback_ into a local variable), then call op->Clear()/Free(), and finally invoke the saved callback; alternatively change ReopenOperationData::Clear() to not null callback_ (but prefer copying the callback in the caller to avoid changing object lifecycle semantics).
🤖 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.
Duplicate comments:
In `@store_handler/data_store_service_client.cpp`:
- Around line 689-699: The ReopenPartition call is always sent to the local
data_store_service_ even when shard_id is owned remotely; add the same
local/remote split used by Read/BatchWriteRecords/DeleteRange/ScanNext: check
IsLocalShard(shard_id) and if true call
data_store_service_->ReopenPartition(..., std::move(callback)); otherwise route
the request to the shard owner via the RPC path (e.g. invoke the existing RPC
client used by other methods or a ReopenPartition RPC that forwards the same
arguments and bridges the callback), ensuring the shard ownership resolution
(shard_id → owner node) and callback forwarding match the patterns in
Read/BatchWriteRecords to deliver results back to the caller.
In `@store_handler/eloq_data_store_service/eloq_store_data_store.cpp`:
- Around line 66-70: ReopenOperationData::Clear() currently nulls callback_ and
calls Free(), so callers that inspect op->callback_ after op->Clear() get a
use-after-free; fix the caller that handles ExecAsyn() failure by capturing the
callback before clearing the op (e.g., store op->callback_ into a local
variable), then call op->Clear()/Free(), and finally invoke the saved callback;
alternatively change ReopenOperationData::Clear() to not null callback_ (but
prefer copying the callback in the caller to avoid changing object lifecycle
semantics).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 629044a6-4569-4e65-8bcc-deda24b8533e
📒 Files selected for processing (13)
store_handler/data_store_service_client.cppstore_handler/data_store_service_client.hstore_handler/eloq_data_store_service/data_store.hstore_handler/eloq_data_store_service/data_store_service.cppstore_handler/eloq_data_store_service/data_store_service.hstore_handler/eloq_data_store_service/eloq_store_data_store.cppstore_handler/eloq_data_store_service/eloq_store_data_store.hstore_handler/eloq_data_store_service/eloqstoretx_service/include/cc/cc_shard.htx_service/include/cc/object_cc_map.htx_service/include/store/data_store_handler.htx_service/src/cc/cc_req_misc.cpptx_service/src/cc/cc_shard.cpp
🚧 Files skipped from review as they are similar to previous changes (11)
- store_handler/eloq_data_store_service/eloqstore
- store_handler/eloq_data_store_service/data_store.h
- tx_service/include/store/data_store_handler.h
- store_handler/eloq_data_store_service/eloq_store_data_store.h
- store_handler/eloq_data_store_service/data_store_service.h
- tx_service/src/cc/cc_req_misc.cpp
- store_handler/eloq_data_store_service/data_store_service.cpp
- tx_service/include/cc/cc_shard.h
- store_handler/data_store_service_client.h
- tx_service/src/cc/cc_shard.cpp
- tx_service/include/cc/object_cc_map.h
thweetkomputer
force-pushed
the
feat-committs-gap-trigger-reopen-zc
branch
from
June 10, 2026 06:58
19fa12b to
6d248a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@store_handler/eloq_data_store_service/eloqstore`:
- Line 1: The CI/dev environment is missing the eloqstore submodule commit
cfbace01d6e45655b48dc540fe02b1ce79a38137 so we cannot validate that
::eloqstore::ReopenRequest, the delayed reopen priority-heap queue, and
ExecAsyn/KvRequest compatibility exist; fetch and initialize the submodule at
that exact SHA (git submodule update --init --recursive and fetch the missing
commit or add the commit to the submodule remote), then re-run the build/tests
and re-inspect the eloqstore API to confirm ReopenRequest and the delayed reopen
queue are present and that EloqStoreDataStore::ReopenPartition still compiles
against KvRequest/ExecAsyn without API breaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3f8f556-2762-4270-8f68-3c4c07dd2e2a
📒 Files selected for processing (13)
store_handler/data_store_service_client.cppstore_handler/data_store_service_client.hstore_handler/eloq_data_store_service/data_store.hstore_handler/eloq_data_store_service/data_store_service.cppstore_handler/eloq_data_store_service/data_store_service.hstore_handler/eloq_data_store_service/eloq_store_data_store.cppstore_handler/eloq_data_store_service/eloq_store_data_store.hstore_handler/eloq_data_store_service/eloqstoretx_service/include/cc/cc_shard.htx_service/include/cc/object_cc_map.htx_service/include/store/data_store_handler.htx_service/src/cc/cc_req_misc.cpptx_service/src/cc/cc_shard.cpp
🚧 Files skipped from review as they are similar to previous changes (12)
- store_handler/eloq_data_store_service/data_store_service.h
- tx_service/include/cc/cc_shard.h
- store_handler/eloq_data_store_service/eloq_store_data_store.h
- tx_service/src/cc/cc_req_misc.cpp
- store_handler/data_store_service_client.cpp
- store_handler/eloq_data_store_service/data_store.h
- store_handler/eloq_data_store_service/data_store_service.cpp
- store_handler/eloq_data_store_service/eloq_store_data_store.cpp
- tx_service/src/cc/cc_shard.cpp
- tx_service/include/cc/object_cc_map.h
- tx_service/include/store/data_store_handler.h
- store_handler/data_store_service_client.h
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tx_service/src/cc/cc_req_misc.cpp`:
- Around line 893-908: The reopen fetch (ccs.FetchRecord(..., reopen=true)) is
invoked while the current in-flight entry cce_ is still present in
fetch_record_reqs_, causing the call to be coalesced into the existing request;
move the reopen FetchRecord call so it occurs after you remove/erase cce_ from
fetch_record_reqs_ (the code that removes the LruEntry pointer), ensuring
FetchRecord sees no existing entry and will dispatch a new reopen fetch. Keep
the same parameters (table_name_, table_schema_, tx_key_.Clone(), cce_,
cc_ng_id_, cc_ng_term_, requester, partition_id_, fetch flags) but invoke
FetchRecord only after the removal of cce_ from fetch_record_reqs_.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ed91a71-b5de-41d6-b645-b8abe9b1f5e0
📒 Files selected for processing (15)
store_handler/data_store_service_client.cppstore_handler/data_store_service_client.hstore_handler/data_store_service_client_closure.hstore_handler/eloq_data_store_service/data_store_service.cppstore_handler/eloq_data_store_service/data_store_service.hstore_handler/eloq_data_store_service/ds_request.protostore_handler/eloq_data_store_service/eloq_store_data_store.cppstore_handler/eloq_data_store_service/eloq_store_data_store.hstore_handler/eloq_data_store_service/eloqstorestore_handler/eloq_data_store_service/internal_request.htx_service/include/cc/cc_req_misc.htx_service/include/cc/cc_shard.htx_service/include/cc/object_cc_map.htx_service/src/cc/cc_req_misc.cpptx_service/src/cc/cc_shard.cpp
💤 Files with no reviewable changes (1)
- store_handler/eloq_data_store_service/eloq_store_data_store.h
✅ Files skipped from review due to trivial changes (1)
- store_handler/eloq_data_store_service/eloqstore
🚧 Files skipped from review as they are similar to previous changes (1)
- tx_service/include/cc/object_cc_map.h
thweetkomputer
force-pushed
the
feat-committs-gap-trigger-reopen-zc
branch
from
June 10, 2026 16:52
986bc1e to
54509aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@store_handler/eloq_data_store_service/eloq_store_config.cpp`:
- Around line 131-133: The flag eloq_store_auto_reopen_pending_time_sec is
declared with DEFINE_int32 allowing negative values that later get cast to
uint64_t and wrap; change its declaration to use DEFINE_uint32 so the flag is an
unsigned 32-bit value and negative inputs are rejected—update the
DEFINE_int32(...) for eloq_store_auto_reopen_pending_time_sec to
DEFINE_uint32(...) in eloq_store_config.cpp to match the other duration/count
flags and prevent incorrect microsecond computations where this value is cast
and multiplied.
In `@tx_service/src/cc/cc_req_misc.cpp`:
- Around line 911-924: The code computes should_reopen by calling
cce_->HasBufferedCommandList() before verifying the CC entry is valid; move the
evaluation and any use of cce_ (including should_reopen and initialization of
reopen_cce/reopen_key/reopen_table_name/reopen_table_schema/reopen_cc_ng_id/reopen_cc_ng_term/reopen_partition_id)
inside the validity guard that checks lock_->GetCcEntry() != nullptr (the block
that ensures the CC entry is valid) so cce_ is never dereferenced when the entry
may be invalidated; keep the reopen_* variables but only populate them (and call
tx_key_.Clone()) within that guarded block and default/unset them outside it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c3fb417-af00-42b6-9245-3c2c7c36bd3e
📒 Files selected for processing (4)
store_handler/eloq_data_store_service/eloq_store_config.cppstore_handler/eloq_data_store_service/eloqstoretx_service/include/cc/object_cc_map.htx_service/src/cc/cc_req_misc.cpp
✅ Files skipped from review due to trivial changes (1)
- store_handler/eloq_data_store_service/eloqstore
thweetkomputer
force-pushed
the
feat-committs-gap-trigger-reopen-zc
branch
from
June 10, 2026 17:15
2ee34ac to
332fb58
Compare
thweetkomputer
force-pushed
the
feat-committs-gap-trigger-reopen-zc
branch
2 times, most recently
from
July 2, 2026 09:57
8cfc5de to
7099f8b
Compare
Collaborator
Author
|
@liangjchen Could you review this PR? |
Contributor
sure, I'll take a look. |
liangjchen
reviewed
Jul 3, 2026
thweetkomputer
force-pushed
the
feat-committs-gap-trigger-reopen-zc
branch
2 times, most recently
from
July 3, 2026 15:36
5729651 to
c52d253
Compare
liangjchen
reviewed
Jul 4, 2026
- Pass the read key as string_view unconditionally: the key owner (FetchRecordCc::tx_key_ locally, the server-side protobuf remotely) outlives the eloqstore request even while it is parked for reopen - Reuse look_key instead of KeyOfEntry in KeyObjectStandbyForwardCc - Drop the TxKey clone before FetchRecord: the DSS client clones non-owner keys synchronously before the call returns - Only access cce_ buffered commands inside the cc entry validity guard
Drops exactly one standby forward message and advances the sequence id as if it were sent, so later messages still flow and the standby sees a permanent gap on the key. Used to verify that buffered commands on the standby are recovered through partition reopen after the primary checkpoints. Self-disarms after the first hit.
- Restore the comment line in FetchRecordCc::Execute that was dropped by accident, leaving a dangling continuation. - Factor the two FetchRecord call sites in ObjectCcMap::Execute(ReplayLogCc &) into a shared fetch_record() lambda, reusing the existing key and key_hash locals instead of recomputing them via KeyOfEntry()/Hash(). - Drop the redundant TxKey::Clone() at the FetchRecord call sites. DataStoreServiceClient promotes a non-owning key to owning before any async use, so the caller-side clone is unnecessary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Execute(KeyObjectStandbyForwardCc &) fetched the record with Sharder::Instance().StandbyNodeTerm(), but the request's own term comes from KeyObjectStandbyForwardCc::ValidTermCheck(), which falls back to CandidateStandbyNodeTerm() when StandbyNodeTerm() is negative. On a candidate standby the two therefore disagree: the fetch was issued with a negative term, FetchRecordCc::ValidTermCheck() then rejected it and aborted the requesters, so the fetch was silently dropped. Use req.StandbyNodeTerm(), matching the sibling call site in the same function. Also factor both FetchRecord call sites into a shared fetch_record() lambda and hoist the partition id, which is now computed once per request instead of once per call site. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FetchRecordCc::Execute used to snapshot six members onto the stack, erase itself from CcShard::fetch_record_reqs_, and then issue a fresh FetchRecord for the reopen. The erase destroys *this, which is why the snapshot and the key clone were needed, and the erase could not be deferred either: FetchRecord coalesces by cce, so a reopen issued while this request was still registered would have been merged into it and never dispatched. Reuse the existing request instead. It keeps tx_key_ and table_name_ in place, drops the snapshot locals and the clone, removes the self-destruction hazard, and preserves the one-fetch-per-cce invariant of fetch_record_reqs_. Re-arming has to reset what a freshly constructed request would have had: requesters_ (never cleared elsewhere, so the already-woken requesters would be enqueued a second time), the per-attempt result fields, and the fetch mode fields that the previous call passed explicitly - fetch_from_primary, snapshot_read_ts, only_fetch_archives. It also has to take a new pin, since ObjectCcMap::BackFill released the one taken for the original fetch. The dispatch and Retry handling is factored out of CcShard::FetchRecord into SubmitFetchRecord so both paths share it. Behavior outside DATA_STORE_TYPE_ELOQDSS_ELOQSTORE is unchanged: the extraction is pure code motion and the reopen branch compiles out, leaving the original RemoveFetchRecordRequest call. Also fix normal_obj_sz_ accounting in ObjectCcMap::BackFill. Widening its condition from && to || lets the block run for entries that are already loaded and already counted, so the unconditional increment double counted on Normal -> Normal and nothing decremented on Normal -> Deleted, drifting the count upward on every backfill. Snapshot the status first and adjust on the transition, matching how the rest of the file maintains the counter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
should_reopen only means "the buffered commands are still stuck" if they were actually evaluated, which happens in ObjectCcMap::BackFill via TryCommitBufferedCommands. An archives-only fetch backfills through BackFillArchives and never reaches BackFill, so its buffered commands are untouched and their presence says nothing about whether a reopen would help. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thweetkomputer
force-pushed
the
feat-committs-gap-trigger-reopen-zc
branch
from
July 21, 2026 03:56
c52d253 to
2c3f299
Compare
Suppresses the ReloadData that the primary drives through UpdateStandbyCkptTs, while still advancing the standby's ckpt ts. With it armed, freshly checkpointed data can only become visible to a standby through partition reopen, which isolates that path from the primary-driven one when verifying that buffered commands are recovered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two problems with re-arming the fetch request in place. RequesterCount() is what gates dispatch in CcShard::FetchRecord, and clearing requesters_ dropped it to zero while the reopen was still in flight. A request that coalesced onto this one would then have brought the count back to one and submitted the same FetchRecordCc to the data store a second time, giving it two outstanding operations writing the same result fields. The previous shape got the invariant for free, since it built a fresh request and passed nullptr as the requester. Keep a null placeholder to restore it. Reads do not reach this path, because they only fetch while the payload status is unknown and BackFill has already set it, but the buffered-command call sites in ObjectCcMap fetch regardless of status, so a forward message for the same key during the reopen window is enough. The reopen also reused whatever tx_key_ the completed fetch had left behind. Derive it from the entry instead, through a new CcMap::KeyOfEntry that hands out a non-owning key pointing into the entry's page. The page outlives the fetch because the entry stays pinned until BackFill. It is declared on CcMap rather than on LruPage: CcMap is already polymorphic and there is one per map, whereas LruPage has no virtuals today and gaining a vtable pointer would cost memory on every page. Verified on a primary/standby pair with skip_one_forward_standby_message and standby_skip_ckpt_reload_data armed, so that the dropped command could only be recovered through partition reopen: the standby converged with no ReloadData or HandleGlobalReopenRequest after startup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 21, 2026
liangjchen
approved these changes
Jul 21, 2026
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Jerry Zhao
Here are some reminders before you submit the pull request
fixes eloqdb/tx_service#issue_id./mtr --suite=mono_main,mono_multi,mono_basicSummary by CodeRabbit
New Features
Chores