Fix commands that modifies the objects into Deleted state#256
Conversation
WalkthroughAdds explicit delete tracking via a new ObjectCommandResult::object_deleted_ flag and ExecResult::Delete; changes object post-processing to treat deleted or TTL‑expired objects as retire cases, gate writes for deleted objects, forward delete transitions as overwrite‑retire commands via AddOverWriteCommand, and propagate commit_ts using max() in multiple paths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant TxExec as TxExecution
participant CcMap as ObjectCcMap
participant Standby as StandbyForwardEntry
participant Store as StorageLayer
Client->>TxExec: Execute object op (Write/Delete or TTL event)
TxExec->>CcMap: Apply ObjectCommandOp -> ObjectCommandResult (includes object_deleted_)
CcMap->>CcMap: Determine ttl_expired_ or object_deleted_
alt ttl_expired_ or object_deleted_
CcMap->>Standby: Create retire overwrite command
Standby->>Standby: AddOverWriteCommand(cmd) — assert overwrite, set has_overwrite, clear list, append single serialized cmd
Standby->>Store: Forward overwrite-retire to standby nodes
else modified (not deleted)
CcMap->>TxExec: AddObjectCommand to command set (write logged)
TxExec->>Standby: Forward normal tx command(s)
end
Note right of CcMap: commit_ts := max(commit_ts, underlying.CommitTs())
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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: 2
🧹 Nitpick comments (2)
src/tx_execution.cpp (2)
6729-6821: Single-object path: deletion handling and logging behavior look consistent with intentThe introduction of
object_deletedand the new conditions:
- Using
ttl_expired || object_deletedbefore enqueuingRetireExpiredTTLObjectCommand, and- Skipping the main
AddObjectCommandwhenobject_deletedis true,ensure that a full-object delete results in only the retire/delete command being logged/forwarded, and not an additional “normal” object-modifying command for the same key. This matches the PR goal of emitting a dedicated DEL/retire command to standbys instead of replaying the original command, while still preserving the existing TTL-expiry path for non-deletion commands.
Two minor follow-ups:
- The comment above the
ifstill only mentions TTL expiry; consider updating it so it also covers theobject_deletedcase for future readers.- Please double‑check that
RetireExpiredTTLObjectCommand()indeed generates the correct command for both TTL-expired and explicit-delete cases, and that tests cover: (a) TTL expiry without delete, (b) explicit delete without TTL expiry, and (c) both combined.- if (lock_acquired == LockType::WriteLock) - { - // If the cce is expired before the command is applyed - // Add a retire command before the write command - if (ttl_expired || object_deleted) + if (lock_acquired == LockType::WriteLock) + { + // If the cce is expired before the command is applied or the + // command deletes the object, add a retire/delete command + // before any normal write command. + if (ttl_expired || object_deleted)
7215-7251: Multi-object path: per-key delete semantics aligned with single-object behaviorFor
MultiObjectCommandOp::PostProcess, the new logic:
- Emitting a retire/delete command when
cmd_res.ttl_expired_ || cmd_res.object_deleted_, and- Guarding the subsequent
AddObjectCommandwith!cmd_res.object_deleted_,brings the multi-key flow in line with the single-key flow and prevents logging a normal modifying command for keys whose objects are fully deleted. That should avoid replaying non-applicable commands on standbys while still logging TTL-expiry retire commands where appropriate.
Same minor cleanup and verification notes as in the single-object path:
- The comment above the
if (cmd_res.ttl_expired_ || cmd_res.object_deleted_)block still only talks about TTL expiry; updating it to explicitly mention deletion would reduce confusion.- Please confirm via tests that multi-key operations spanning a mix of (a) normal updates, (b) TTL-expiry, and (c) deletes produce the expected sequence of logged/forwarded commands per key.
- // If the cce is expired before the command is applyed - // Add a retire command before the write command. - // If the cmd will delete the object, just add a delete - // command. - if (cmd_res.ttl_expired_ || cmd_res.object_deleted_) + // If the cce is expired before the command is applied, or the + // command deletes the object, add a retire/delete command for + // this key instead of a normal write command. + if (cmd_res.ttl_expired_ || cmd_res.object_deleted_)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
include/cc/object_cc_map.h(6 hunks)include/command_set.h(1 hunks)include/standby.h(1 hunks)include/tx_command.h(1 hunks)include/tx_operation_result.h(2 hunks)src/standby.cpp(1 hunks)src/tx_execution.cpp(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
include/standby.h (1)
src/standby.cpp (2)
AddOverWriteCommand(53-62)AddOverWriteCommand(53-53)
🔇 Additional comments (11)
include/command_set.h (1)
71-72: Explicit type specification is a minor clarity improvement.The change from
auto&to explicitstd::unordered_map<CcEntryAddr, CmdSetEntry>&makes the type immediately visible without needing to tracetable_it->second. While functionally equivalent, this improves code readability.include/tx_command.h (2)
55-55: Typo fix: "Succes" → "Success".Good catch on the typo.
56-56: All ExecResult values, including Delete, are already properly handled throughout the codebase.The ExecResult enum has six values (Fail, Read, Write, Delete, Block, Unlock), and all are accounted for in the command execution handler at
include/cc/object_cc_map.h:1013-1165:
- Delete and Write are evaluated as modification flags (lines 1024-1025, 1065-1066)
- Block is explicitly checked and returns early (line 1142)
- Unlock is explicitly checked (line 1161)
- Fail and Read are handled implicitly via the default control flow
No changes are needed.
include/tx_operation_result.h (2)
1007-1007: Proper initialization of object_deleted_ in Reset().The new
object_deleted_flag is correctly initialized tofalsein theReset()method, ensuring clean state between operations.
1027-1033: New object_deleted_ field complements object_modified_ for deletion tracking.The addition of
object_deleted_alongsideobject_modified_provides clear semantics:
object_modified_: Command updates the object (write set/log)object_deleted_: Command deletes the objectThis separation enables proper handling of deletion scenarios in standby forwarding and retire command logic.
include/standby.h (1)
48-48: New AddOverWriteCommand method provides clear semantics for overwrite/delete scenarios.The addition of
AddOverWriteCommand(TxCommand *cmd)alongside the existingAddTxCommand(ApplyCc &cc_req)creates a dedicated entry point for overwrite/delete commands. The naming clearly communicates the method's purpose for forwarding retire/overwrite commands to standby nodes.src/standby.cpp (1)
53-62: AddOverWriteCommand implementation correctly enforces overwrite-only semantics.The implementation properly:
- Validates input with
assert(cmd->IsOverwrite())- appropriate for internal API- Sets the
has_overwriteprotocol flag- Clears existing commands via
req.clear_cmd_list()- correct for overwrite semantics where the new command replaces all previous commands- Serializes and appends the overwrite command
The
clear_cmd_list()call is intentional and correct: overwrite commands (including deletions) should replace any accumulated commands rather than append to them.include/cc/object_cc_map.h (4)
132-133: LGTM!Clean addition of
object_deletedtracking alongside the existingobject_modifiedpattern. The binding follows the established convention in this method.
921-926: LGTM!Using
AddOverWriteCommandfor the retire command correctly signals object deletion semantics to standby nodes during TTL expiration handling.
1024-1043: LGTM!The deletion tracking logic correctly identifies when a command results in object deletion and routes the appropriate retire command to standby nodes for cleanup.
1069-1094: LGTM!The logic correctly handles the sequence where TTL reset recovery command may be followed by a deletion retire command, with normal write forwarding as the fallback. The conditional structure properly ensures deleted objects get retire commands forwarded to standby nodes.
MrGuin
force-pushed
the
fix_cmd_delete
branch
from
f23cc07 to
77d1ffb
Compare
| // skipped by checkpointer. | ||
| commit_ts = cce->CommitTs(); | ||
| // Update dirty_commit_ts with the req.CommitTs(). | ||
| commit_ts = std::max(commit_ts, cce->CommitTs()); |
There was a problem hiding this comment.
this should be applied to ReplayLogCc as well
MrGuin
force-pushed
the
fix_cmd_delete
branch
from
77d1ffb to
9601b27
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tx_service/src/tx_execution.cpp (1)
1086-1095: Fix TX_TRACE callsites with wrong identifiers / missing semicolon (won’t compile as-is)There are a few obvious TX_TRACE issues that will break compilation or logging:
- Line 1086–1095,
ProcessTxRequest(ReloadCacheTxRequest &req): the trace macro passes&fi_req, which is not in scope; this should be&req.- Line 7768–7774,
PostProcess(BatchReadOperation &batch_read_op): the trace macro passes&read, which is not in scope; this should be&batch_read_op.- Line 7530–7541,
PostProcess(KickoutDataAllOp &kickout_data_all_op): the lambda’sreturn ...append(std::to_string(this->tx_term_))line is missing the terminating semicolon, unlike other TX_TRACE lambdas.Suggested fix:
void TransactionExecution::ProcessTxRequest(ReloadCacheTxRequest &req) { - TX_TRACE_ACTION_WITH_CONTEXT( - this, - &fi_req, + TX_TRACE_ACTION_WITH_CONTEXT( + this, + &req, [this]() -> std::string { return std::string("\"tx_number\":") .append(std::to_string(this->TxNumber())) .append("\"tx_term\":") .append(std::to_string(this->tx_term_)); }); ... }void TransactionExecution::PostProcess(BatchReadOperation &batch_read_op) { - TX_TRACE_ACTION_WITH_CONTEXT( - this, - &read, + TX_TRACE_ACTION_WITH_CONTEXT( + this, + &batch_read_op, [this]() -> std::string { return std::string("\"tx_number\":") .append(std::to_string(this->TxNumber())) .append("\"tx_term\":") .append(std::to_string(this->tx_term_)); }); ... }void TransactionExecution::PostProcess(KickoutDataAllOp &kickout_data_all_op) { TX_TRACE_ACTION_WITH_CONTEXT( this, &kickout_data_all_op, [this]() -> std::string { return std::string("\"tx_number\":") .append(std::to_string(this->TxNumber())) - .append("\"tx_term\":") - .append(std::to_string(this->tx_term_)) - }); + .append("\"tx_term\":") + .append(std::to_string(this->tx_term_)); + }); state_stack_.pop_back(); }These are critical to fix even if they predate this PR, otherwise the translation unit will not compile.
Also applies to: 7530-7541, 7768-7774
♻️ Duplicate comments (1)
tx_service/include/cc/object_cc_map.h (1)
1065-1067: ConvertLOG(INFO)toDLOG(INFO)to avoid production log spam.This logging statement executes for every object modification and appears to be debug logging.
Apply this diff:
object_deleted = exec_rst == ExecResult::Delete; object_modified = object_deleted || exec_rst == ExecResult::Write; - LOG(INFO) << "object_deleted: " << object_deleted; + DLOG(INFO) << "object_deleted: " << object_deleted;
🧹 Nitpick comments (2)
tx_service/include/tx_command.h (1)
51-59: Consider pinning explicitExecResultnumeric values to avoid ABI/wire changes
ExecResult::Deleteis inserted betweenWriteandBlockwithout explicit underlying values, which shifts the numeric codes forBlockandUnlock. IfExecResultis ever serialized by value (RPC, logs that are parsed, persisted state, metrics labels by cast, etc.), this becomes a silent protocol/ABI change.To make future additions safe and keep current behavior predictable, consider assigning explicit values and, if needed, preserving historical codes for existing states, e.g.:
enum class ExecResult : uint8_t { Fail = 0, // Failed to execute command Read = 1, // Success to execute readonly command Write = 2, // Success to execute the command and modified object. Block = 3, // The command is blocked Unlock = 4, // There has not expected result and release ccentry lock Delete = 5, // The object will be deleted by the command. };(Or any explicit mapping that matches your existing deployments’ expectations.)
tx_service/include/standby.h (1)
46-50: Minor naming consistency:AddOverWriteCommandvsIsOverwriteNIT: to align with
TxCommand::IsOverwrite(), consider renaming this toAddOverwriteCommand(single word) for consistency and searchability across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tx_service/include/cc/object_cc_map.h(6 hunks)tx_service/include/command_set.h(1 hunks)tx_service/include/standby.h(1 hunks)tx_service/include/tx_command.h(1 hunks)tx_service/include/tx_operation_result.h(2 hunks)tx_service/src/standby.cpp(1 hunks)tx_service/src/tx_execution.cpp(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tx_service/include/command_set.h
🧰 Additional context used
🧬 Code graph analysis (1)
tx_service/include/standby.h (1)
tx_service/src/standby.cpp (2)
AddOverWriteCommand(53-62)AddOverWriteCommand(53-53)
🔇 Additional comments (9)
tx_service/include/tx_operation_result.h (1)
998-1033: Delete-tracking flag integration inObjectCommandResultlooks consistentResetting
object_deleted_alongsideobject_modified_and documenting both flags keeps the result object internally coherent and makes delete-aware flows easier to reason about. As you wire this through execution paths, just ensure callers maintain a clear invariant (e.g., whether delete operations should also markobject_modified_or rely solely onobject_deleted_for logging/forwarding decisions).tx_service/src/standby.cpp (1)
53-61:AddOverWriteCommandenforces a single-command overwrite; confirm this matches all callsites’ expectations
AddOverWriteCommandassertscmd->IsOverwrite(), setshas_overwrite, and clearscmd_listbefore serializing the command. That effectively discards any commands previously queued on thisStandbyForwardEntryand makes the overwrite the sole forwarded command.If the intent is “this overwrite/delete supersedes any earlier buffered commands for the object,” this is perfect. If there are code paths that may call
AddTxCommand()first (e.g., buffer non-overwrite ops) and then add an overwrite/delete that should logically coexist in the same forward batch, those earlier commands will be silently dropped.Might be worth double-checking the new callsites to ensure this “clear then append” semantics are always desired.
tx_service/src/tx_execution.cpp (2)
6767-6846: Object deletion handling inPostProcess(ObjectCommandOp)looks consistentUsing
object_deletedto (a) include deletes in thettl_expired || object_deletedretire path and (b) skip adding the primary command tocmd_set_when the object is deleted ensures that WAL/forwarding only sees the canonical delete/retire command, which aligns with the new delete semantics. No changes needed here.
7261-7301: Multi-object delete semantics mirror single-object path correctlyExtending the retire path to
cmd_res.ttl_expired_ || cmd_res.object_deleted_and guarding the mainAddObjectCommandwith!cmd_res.object_deleted_provides the same “log only the canonical delete/retire command” behavior for multi-object commands as for single-object, while still preserving TTL-expired handling and OCC checks. This looks correct.tx_service/include/cc/object_cc_map.h (5)
130-134: LGTM!The addition of
object_deletedreference follows the existing pattern forobject_modifiedand properly exposes the new field for tracking delete transitions.
921-926: LGTM!Using
AddOverWriteCommandfor TTL-expired cleanup ensures standby nodes properly clear the object state via the overwrite flow.
1024-1043: LGTM!The delete detection and conditional forwarding logic correctly routes delete operations through
AddOverWriteCommandwhile continuing to useAddTxCommandfor non-delete modifications.
1074-1094: Verify: Canttl_resetandobject_deletedboth be true?If a command both triggers TTL reset (line 1074) and deletes the object (line 1083), both
AddOverWriteCommand(recover_command)andAddOverWriteCommand(retire_command.get())will execute. This would forward a recover command followed immediately by a delete command, which seems redundant.Consider whether
object_deletedshould skip the TTL reset recovery path, or if this combination is intentionally allowed for correctness on standby nodes.
2005-2017: LGTM!Taking the maximum of
commit_tsandcce->CommitTs()ensures that when buffered commands advance the entry beyond the incoming request's timestamp, the dirty tracking correctly reflects the actual committed version. This prevents CcPages with buffered commands from being skipped during SnapshotSync.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tx_service/src/tx_execution.cpp (1)
7246-7305: Assert guarding needs refinement to match single-object path behaviorIn the multi-object
PostProcesspath, the assert at line 7269 is guarded byttl_expired_ || object_deleted_:if (cmd_res.ttl_expired_ || cmd_res.object_deleted_) { assert(cmd_res.ttl_ == UINT64_MAX); // ... }However, the single-object
PostProcesspath (line 6795+) does not include this assert in the equivalent block—it only assertsttl == UINT64_MAXwithin thettl_resetcondition (line 6816). This inconsistency suggests the assert in the multi-object path may be overly strict:
If
object_deleted_can be true withttl_values other thanUINT64_MAX, this assert will break debug builds. The execution layer should either:
- Restrict the assert to TTL-expiry only:
orif (cmd_res.ttl_expired_) { assert(cmd_res.ttl_ == UINT64_MAX); }- Ensure deletes always carry
ttl_ == UINT64_MAXbefore reaching this point.The comment "add a retire/delete command for this key instead of a normal write command" is now misleading. When
ttl_expired_ == true && object_deleted_ == false, a normal command is still added. Clarify that TTL expiry adds both retire and normal commands, while explicit deletes add only the retire command.tx_service/include/cc/object_cc_map.h (1)
1073-1093: Bothttl_reset_andobject_deletedcan execute simultaneously, sending contradictory commands to standby nodes.The current structure allows both the
ttl_reset_block (line 1073) and theobject_deletedblock (line 1082) to execute in the same code path because the second is an independentif-elserather thanelse-if. When both conditions are true, standby nodes receive bothRecoverTTLObjectCommand(line 1079) andRetireExpiredTTLObjectCommand(line 1088)—operations that semantically contradict each other.Restructure to enforce mutual exclusivity:
if (obj_result.ttl_reset_) { auto recover_command = cmd->RecoverTTLObjectCommand(); forward_entry->AddOverWriteCommand(recover_command); } else if (object_deleted) { auto retire_command = cmd->RetireExpiredTTLObjectCommand(); forward_entry->AddOverWriteCommand(retire_command.get()); } else { forward_entry->AddTxCommand(req); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tx_service/include/cc/object_cc_map.h(6 hunks)tx_service/src/tx_execution.cpp(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tx_service/include/cc/object_cc_map.h (1)
tx_service/include/tx_command.h (1)
ExecResult(51-260)
🔇 Additional comments (3)
tx_service/src/tx_execution.cpp (1)
6753-6850: Delete-aware single-object command handling looks correct; please verifyRetireExpiredTTLObjectCommand()semanticsThe new
object_deletedflag and the updated logic in the write-lock branch do what you want:
- For
ttl_expired || object_deleted, you emit a retire/delete command viaRetireExpiredTTLObjectCommand()before any “normal” write.- For deleted objects, you now skip the normal
AddObjectCommandentirely (if (!object_deleted)), so the log/forward path won’t see a non-DEL command for a deleted object.- For non-deleting writes with TTL expiry, you still log both the retire command and the real command, which preserves the old TTL semantics.
This matches the PR’s goal of making deletes explicit and avoiding writing normal object commands for already-deleted objects.
One thing to double-check:
RetireExpiredTTLObjectCommand()is now used for both TTL expiry and logical deletes. That helper must generate the appropriate DEL/retire representation for delete-causing commands (e.g., emptySETRANGE) so that standbys and snapshot sync see a proper delete, not just a TTL-retire pattern. If it still assumes “TTL expired” only, you may need to branch inside that helper based on the newExecResult::Delete/object_deleted_semantics.tx_service/include/cc/object_cc_map.h (2)
133-133: LGTM!The object_deleted reference extraction follows the same pattern as object_modified and enables clean tracking of deletion state throughout the Execute method.
1078-1079: No action needed — the calling patterns are correct and necessary.Line 1079 passes
recover_commanddirectly toAddOverWriteCommand, while lines 925, 1036, and 1088 use.get(). This is not an inconsistency:RecoverTTLObjectCommand()returnsTxCommand*(raw pointer), whileRetireExpiredTTLObjectCommand()returnsstd::unique_ptr<TxCommand>. SinceAddOverWriteCommandexpectsTxCommand*, line 1079 correctly passes the raw pointer directly, and the other lines correctly extract the raw pointer from the unique_ptr using.get(). The different calling patterns are appropriate given the different return types.Likely an incorrect or invalid review comment.
MrGuin
force-pushed
the
fix_cmd_delete
branch
from
173a8aa to
a9319bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tx_service/include/cc/object_cc_map.h (1)
2410-2418: Verify timestamp consistency withKeyObjectStandbyForwardCc.This is a follow-up to the past review comment.
KeyObjectStandbyForwardCc(line 2007) now usesstd::max(commit_ts, cce->CommitTs())before updatinglast_dirty_commit_ts_to account for buffered commands that may have advanced the entry's timestamp.However,
ReplayLogCcat lines 2412-2418 still uses the originalcommit_tsdirectly. AfterEmplaceAndCommitBufferedTxnCommand(line 2379),cce->CommitTs()may exceedcommit_ts, potentially causinglast_dirty_commit_ts_to be set to a stale value.If the behavior is intentionally different for replay scenarios, a brief comment explaining why would be helpful.
🧹 Nitpick comments (1)
tx_service/include/standby.h (1)
46-49: ClarifyAddOverWriteCommand’s destructive contract in the API.From the implementation, this method asserts
cmd->IsOverwrite(), setshas_overwrite, clears any existingcmd_list, and then appends a single serialized command. That “single overwrite that discards prior commands” contract isn’t obvious from the declaration alone.Consider:
- Adding a brief comment here documenting that this replaces any previously queued commands for the entry.
- Optionally normalizing the spelling to
AddOverwriteCommandfor consistency withIsOverwrite().Based on relevant code snippet in
tx_service/src/standby.cpp.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tx_service/include/cc/object_cc_map.h(6 hunks)tx_service/include/command_set.h(1 hunks)tx_service/include/standby.h(1 hunks)tx_service/include/tx_command.h(1 hunks)tx_service/include/tx_operation_result.h(2 hunks)tx_service/src/standby.cpp(1 hunks)tx_service/src/tx_execution.cpp(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tx_service/include/tx_operation_result.h
🧰 Additional context used
🧠 Learnings (1)
📚 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/include/cc/object_cc_map.h
🧬 Code graph analysis (1)
tx_service/include/standby.h (1)
tx_service/src/standby.cpp (2)
AddOverWriteCommand(53-62)AddOverWriteCommand(53-53)
🔇 Additional comments (10)
tx_service/include/command_set.h (1)
70-72: Explicit map type binding inAddObjectCommandlooks good.Using an explicit
std::unordered_map<CcEntryAddr, CmdSetEntry> &instead ofauto &is a no-op behaviorally and improves readability/tooling; no issues here.tx_service/include/tx_command.h (1)
51-59: No action required—ExecResult::Deleteis already properly integrated.The new
Deleteenum member is already fully handled throughout the codebase. All existing references toExecResultuse safe enum comparisons (not numeric ordinals), and theDeletecase is correctly handled inobject_cc_map.h(lines 1024–1025, 1065–1066), where deletion is detected and triggers appropriate retirement command logic. No serialization dependencies or switch statements overExecResultwere found.tx_service/src/standby.cpp (1)
53-62: AddOverWriteCommand correctly enforces overwrite semantics for TTL-expiry and recovery operations.The implementation is appropriate and well-guarded:
- All four call sites pass commands from
RetireExpiredTTLObjectCommand()orRecoverTTLObjectCommand()methods—factory methods specifically designed for overwrite scenarios.- Call contexts confirm usage only during TTL expiry handling ("Forward retire command to standby node to clear the object").
assert(cmd->IsOverwrite())provides development-time validation.clear_cmd_list()correctly discards prior buffered commands, contrasting withAddTxCommandwhich appends without clearing.The destructive semantics align with intended usage: retire and recover operations should supersede any prior commands for the object in standby forwarding.
tx_service/src/tx_execution.cpp (2)
6753-6847: Delete-aware logging inPostProcess(ObjectCommandOp)looks correctUsing
object_deletedto (a) emit a retire/delete command when the command deletes the object, and (b) suppress the normalAddObjectCommandfor that key ensures that WAL/forwarding only replays a delete for logically deleted objects, while still handling TTL-expiry viattl_expired. This matches the PR intent and avoids replaying a stale “normal” command on a key that no longer exists.
7246-7300: Confirmttl_invariant now also holds for deleted objects in multi-object pathThe condition was broadened from
cmd_res.ttl_expired_tocmd_res.ttl_expired_ || cmd_res.object_deleted_, but the existing assertionassert(cmd_res.ttl_ == UINT64_MAX);remains. This now assumes that for any explicit delete (object_deleted_ == true),ttl_is also set toUINT64_MAX.If the execution layer ever sets
object_deleted_ == truewhile leavingttl_at some other value (e.g., the prior TTL), this assertion will start firing for valid deletes. Please double‑check that theObjectCommandResultproducer setsttl_ == UINT64_MAXfor all delete cases, or narrow the assertion to only thettl_expired_case if that invariant is not guaranteed.tx_service/include/cc/object_cc_map.h (5)
130-133: LGTM!Clean addition of the
object_deletedreference binding following the established pattern forobject_modified.
921-926: LGTM!Using
AddOverWriteCommandfor TTL-expired retire commands correctly implements the delete-forward semantics required by the PR objectives.
1024-1043: LGTM!The delete detection via
ExecResult::Deleteand conditional forwarding logic correctly implements the PR objective of forwarding delete commands to standby nodes using overwrite semantics.
2004-2016: LGTM!Using
std::max(commit_ts, cce->CommitTs())correctly handles the case where buffered commands may have advanced the entry's commit timestamp beyond the request's originalcommit_ts, ensuring dirty pages are not skipped by SnapshotSync.
1078-1093: No issue found.RecoverTTLObjectCommand()returns a raw pointer (TxCommand*) whileRetireExpiredTTLObjectCommand()returnsstd::unique_ptr<TxCommand>. The usage at line 1079 (direct pass) and line 1088 (.get()call) are both correct for their respective return types.Likely an incorrect or invalid review comment.
MrGuin
force-pushed
the
fix_cmd_delete
branch
from
a9319bc to
aa28e2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tx_service/src/standby.cpp (1)
53-62: Clarify the implementation of factory methods that feed intoAddOverWriteCommand.The factory methods
RetireExpiredTTLObjectCommand()andRecoverTTLObjectCommand()called at all callsites have only base class implementations thatassert(false)and returnnullptr. No overrides exist in derived command classes. This means:
- The
assert(cmd->IsOverwrite())at line 55 is unreachable in current form—calls would crash with the factory method'sassert(false)first.- The destructive behavior (line 58 clears the command list) is intentional, as confirmed by the if/else logic at callsites (callsites distinguish between delete vs. non-delete paths).
Either provide implementations of these factory methods that return properly-typed overwrite commands, or clarify if these code paths are incomplete/not yet exercised.
🧹 Nitpick comments (1)
tx_service/include/cc/object_cc_map.h (1)
1798-1807: Duplicatelast_dirty_commit_ts_update inUploadTxCommandsCc.This block updates
last_dirty_commit_ts_twice with the same condition and value:if (commit_ts > last_dirty_commit_ts_) { last_dirty_commit_ts_ = commit_ts; } if (commit_ts > last_dirty_commit_ts_) { last_dirty_commit_ts_ = commit_ts; }The second
ifis redundant and can be safely removed to reduce noise.- if (commit_ts > last_dirty_commit_ts_) - { - last_dirty_commit_ts_ = commit_ts; - } if (commit_ts > last_dirty_commit_ts_) { last_dirty_commit_ts_ = commit_ts; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tx_service/include/cc/object_cc_map.h(6 hunks)tx_service/include/command_set.h(1 hunks)tx_service/include/standby.h(1 hunks)tx_service/include/tx_command.h(1 hunks)tx_service/include/tx_operation_result.h(2 hunks)tx_service/src/standby.cpp(1 hunks)tx_service/src/tx_execution.cpp(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tx_service/include/command_set.h
- tx_service/include/tx_operation_result.h
🧰 Additional context used
🧠 Learnings (1)
📚 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/include/cc/object_cc_map.h
🧬 Code graph analysis (2)
tx_service/include/cc/object_cc_map.h (1)
tx_service/include/tx_command.h (1)
ExecResult(51-260)
tx_service/include/standby.h (1)
tx_service/src/standby.cpp (2)
AddOverWriteCommand(53-62)AddOverWriteCommand(53-53)
🔇 Additional comments (6)
tx_service/include/tx_command.h (1)
53-58: No switch statements on ExecResult exist in the codebase—only if/comparison statements.The enum adds Delete as expected, and it's already properly handled in all existing ExecResult usage locations (object_cc_map.h lines 1024–1025 and 1066–1067). The verification request in the original comment cannot be applied as stated since there are no switch statements to verify.
tx_service/include/standby.h (1)
48-48: All callsites of the renamed method have been properly updated.The breaking API change is correctly implemented. The method signature changed from
AddTxCommand(TxCommand *cmd)toAddOverWriteCommand(TxCommand *cmd), and all callsites using theTxCommand*version have been updated. The remainingAddTxCommandcalls in the codebase reference a different overload—AddTxCommand(ApplyCc &cc_req)—which was not renamed and correctly remains unchanged.tx_service/src/tx_execution.cpp (2)
6754-6847: Deleted-object handling in single-object post-processing looks correctUsing
object_deletedto (a) emit a retire/delete command whenttl_expired || object_deletedand (b) gate the normalAddObjectCommandwhen!object_deletedmatches the intended semantics: deleted objects no longer generate a regular overwrite entry but still get a DEL/retire log entry and forward path. This keeps TTL-only behavior unchanged while covering explicit deletes as well.
7263-7300: Multi-object delete/TTL retire behavior is consistent and safeThe multi-object path now mirrors the single-object post-processing: for each key, a retire/delete command is added when
ttl_expired_ || object_deleted_, and the normalAddObjectCommandis skipped whenobject_deleted_is true. This avoids logging regular overwrites for deleted objects while still capturing delete transitions for logging and standby forwarding, without changing the existing TTL-expiration behavior.tx_service/include/cc/object_cc_map.h (2)
914-927: TTL‑expired forwarding now uses overwrite semantics – behavior looks correct.The change to forward TTL‑retire operations via
AddOverWriteCommand(retire_command.get())in the ttl‑expired branch aligns with the overwrite‑first semantics used elsewhere and ensures standbys see a clear delete transition when the primary treats the object as expired. This matches the PR goal of explicitly propagating delete‑state transitions.
2006-2018: Usingstd::max(commit_ts, cce->CommitTs())for dirty timestamps is appropriate.Updating
commit_tswithstd::max(commit_ts, cce->CommitTs())before writinglast_dirty_commit_ts_andccp->last_dirty_commit_ts_ensures the dirty watermark never lags behind the CCEntry’s actual version, even when buffered commands advancecce->CommitTs()beyond the incoming forward message’sCommitTs(). This is consistent with the goal of preventing SnapshotSync/checkpointer from skipping pages that still have buffered or newly applied commands.
| ObjectCommandResult &obj_result = hd_res->Value(); | ||
| CcEntryAddr &cce_addr = obj_result.cce_addr_; | ||
| bool &object_modified = obj_result.object_modified_; | ||
| bool &object_deleted = obj_result.object_deleted_; | ||
| CcEntry<KeyT, ValueT, false, false> *cce = nullptr; |
There was a problem hiding this comment.
Fix AddOverWriteCommand call with std::unique_ptr and confirm delete tracking semantics.
The new object_deleted tracking and use of ExecResult::Delete look consistent with the intent (treat delete-state transitions as modifications and drive overwrite/delete forwarding). However, there is a correctness bug in the TTL‑reset forward path:
- Line 1079 creates a
std::unique_ptr<TxCommand>:auto recover_command = cmd->RecoverTTLObjectCommand();
- Line 1080 calls:
forward_entry->AddOverWriteCommand(recover_command);
Given the other call sites use a raw pointer (retire_command.get()), AddOverWriteCommand is very likely declared as taking a TxCommand*. Passing a std::unique_ptr<TxCommand> here will not compile (no implicit conversion), or, if there is an overload taking a std::unique_ptr, it should be passed with std::move to transfer ownership.
Update this call to match the other site:
- auto recover_command = cmd->RecoverTTLObjectCommand();
- forward_entry->AddOverWriteCommand(recover_command);
+ auto recover_command = cmd->RecoverTTLObjectCommand();
+ if (recover_command) {
+ forward_entry->AddOverWriteCommand(recover_command.get());
+ }This both fixes the type mismatch and guards against a possible null recover command.
Separately, the revised object_deleted / object_modified logic around ExecResult::Delete looks correct and should ensure deletes are treated as modifications for logging, forwarding, and lock release callbacks.
Also applies to: 1014-1095
🤖 Prompt for AI Agents
In tx_service/include/cc/object_cc_map.h around lines 1014 to 1095 (specifically
1079–1080 and similar sites), the call to AddOverWriteCommand passes a
std::unique_ptr<TxCommand> (recover_command) directly which mismatches the other
call sites that pass a raw pointer and may not compile or transfer ownership
correctly; change the call to pass the raw pointer (recover_command.get()) to
match existing usage and add a null check before calling AddOverWriteCommand to
guard against a null recover command; alternatively, if AddOverWriteCommand is
intended to take ownership, call it with std::move(recover_command) instead—pick
the raw-pointer form to match retire_command.get() unless you intentionally want
ownership transfer, and apply the same fix to all similar occurrences in the
1014–1095 range.
2 participants
Related PR: eloqdata/eloqkv#319
Closes eloqdata/eloqkv#318, https://github.com/eloqdata/project_tracker/issues/72 and https://github.com/eloqdata/project_tracker/issues/82.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.