Skip to content

fix standby forward cc request by adding missed check.#144

Merged
MrGuin merged 1 commit into
mainfrom
fix-standby-forward-zc
Sep 25, 2025
Merged

fix standby forward cc request by adding missed check.#144
MrGuin merged 1 commit into
mainfrom
fix-standby-forward-zc

Conversation

@thweetkomputer

@thweetkomputer thweetkomputer commented Sep 24, 2025

Copy link
Copy Markdown
Collaborator

Here are some reminders before you submit the pull request

Summary by CodeRabbit

  • Bug Fixes
    • Resolved inconsistencies in object counts after add/upload operations; counts now update immediately after commit, eliminating under/over-counting.
    • Ensures in-memory counters stay aligned with the final payload status during batched operations, improving accuracy in displayed totals and metrics.

@coderabbitai

coderabbitai Bot commented Sep 24, 2025

Copy link
Copy Markdown

Walkthrough

Synchronizes in-memory counter normal_obj_sz_ with post-commit payload status after EmplaceAndCommitBufferedTxnCommand in two code paths (Emplace and UploadTxCommands). Recomputes new_status from cce, then increments or decrements normal_obj_sz_ based on prior existence and resulting Normal/non-Normal status.

Changes

Cohort / File(s) Summary
In-memory counter sync for payload status
include/cc/object_cc_map.h
After EmplaceAndCommitBufferedTxnCommand in Emplace and UploadTxCommands, recompute payload status (new_status) and adjust normal_obj_sz_: decrement if existed and becomes non-Normal; increment if new and becomes Normal. No API signature changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller as Emplace / UploadTxCommands
  participant Map as ObjectCcMap
  participant Txn as TxnBuffer
  participant Commit as CommitLogic

  Caller->>Map: Prepare cce and command
  Map->>Txn: EmplaceAndCommitBufferedTxnCommand(cmd)
  Txn-->>Commit: Batch/commit
  Commit-->>Map: Commit result
  Note over Map: Recompute new_status from cce
  alt Previously existed AND new_status != Normal
    Map->>Map: normal_obj_sz_--
  else Previously not exist AND new_status == Normal
    Map->>Map: normal_obj_sz_++
  end
  Map-->>Caller: Return (post-commit, in-memory count synced)
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • liunyl

Poem

A carrot-counting hare, precise and bright,
Tweaks the numbers after every write—
If payloads bloom, the tally grows,
If petals fade, it gently slows.
Thump-thump! The map now keeps in time,
With commits’ steps in perfect rhyme. 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The PR title "fix standby forward cc request by adding missed check." describes fixing a missed check for standby forward cc requests, but the provided changes (modifying include/cc/object_cc_map.h to keep normal_obj_sz_ in sync after EmplaceAndCommitBufferedTxnCommand) do not match that description, so the title is misleading and does not summarize the main change in the diff. Rename the PR to a concise title that reflects the actual change (for example, "Sync normal_obj_sz_ with payload status after buffered commit in object_cc_map.h") and ensure the PR description and title consistently describe the primary change and affected component.
Description Check ⚠️ Warning The PR description only contains the generic checklist template and a link to an issue but lacks a concise summary of the actual code changes, rationale, testing details, documentation links, or confirmation that the checklist items (tests, docs, CI) are completed, so the description is incomplete relative to the repository template. Update the PR description to include a short summary of what changed and why, list affected files/behaviors, indicate which tests were added or run and include CI/mtr results, mark checklist items as completed or explain outstanding work, and add the appropriate "fixes eloqdb/tx_service#" reference if this PR resolves the linked issue.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-standby-forward-zc

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
include/cc/object_cc_map.h (2)

1749-1758: Reduce duplication: extract a tiny helper to adjust normal_obj_sz_.

The same pattern appears in multiple places. A small helper reduces drift and future misses.

Apply locally at these sites and elsewhere after introducing a helper:

-            if (s_obj_exist && payload_status != RecordStatus::Normal)
-            {
-                TemplateCcMap<KeyT, ValueT, false, false>::normal_obj_sz_--;
-            }
-            else if (!s_obj_exist && payload_status == RecordStatus::Normal)
-            {
-                TemplateCcMap<KeyT, ValueT, false, false>::normal_obj_sz_++;
-            }
+            AdjustNormalObjCount(s_obj_exist, payload_status)
-                RecordStatus new_status = cce->PayloadStatus();
-                if (s_obj_exist && new_status != RecordStatus::Normal)
-                {
-                    TemplateCcMap<KeyT, ValueT, false, false>::normal_obj_sz_--;
-                }
-                else if (!s_obj_exist && new_status == RecordStatus::Normal)
-                {
-                    TemplateCcMap<KeyT, ValueT, false, false>::normal_obj_sz_++;
-                }
+                RecordStatus new_status = cce->PayloadStatus();
+                AdjustNormalObjCount(s_obj_exist, new_status);

Add this private helper in ObjectCcMap:

private:
    inline void AdjustNormalObjCount(bool existed_before, RecordStatus after) {
        if (existed_before && after != RecordStatus::Normal) {
            --this->normal_obj_sz_;
        } else if (!existed_before && after == RecordStatus::Normal) {
            ++this->normal_obj_sz_;
        }
    }

Also applies to: 1943-1951


1765-1772: Duplicate last_dirty_commit_ts_ update.

The same condition and assignment appear twice back-to-back; remove one.

-            if (commit_ts > last_dirty_commit_ts_)
-            {
-                last_dirty_commit_ts_ = commit_ts;
-            }
             if (commit_ts > ccp->last_dirty_commit_ts_)
             {
                 ccp->last_dirty_commit_ts_ = commit_ts;
             }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2bd068a and 81feb08.

📒 Files selected for processing (2)
  • include/cc/object_cc_map.h (1 hunks)
  • include/tx_record.h (0 hunks)
💤 Files with no reviewable changes (1)
  • include/tx_record.h
🧰 Additional context used
🧬 Code graph analysis (1)
include/cc/object_cc_map.h (2)
include/tx_record.h (1)
  • RecordStatus (36-138)
include/cc/template_cc_map.h (1)
  • normal_obj_sz_ (12210-12212)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
include/cc/object_cc_map.h (1)

1765-1772: Remove duplicate last_dirty_commit_ts_ update

The two consecutive if (commit_ts > last_dirty_commit_ts_) blocks are identical; keep one.

             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

📥 Commits

Reviewing files that changed from the base of the PR and between 81feb08 and 61fcabd.

📒 Files selected for processing (1)
  • include/cc/object_cc_map.h (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
include/cc/object_cc_map.h (2)
include/tx_record.h (1)
  • RecordStatus (36-138)
include/cc/template_cc_map.h (1)
  • normal_obj_sz_ (12210-12212)

Comment thread include/cc/object_cc_map.h
@MrGuin MrGuin merged commit 67a6466 into main Sep 25, 2025
4 checks passed
@MrGuin MrGuin deleted the fix-standby-forward-zc branch September 25, 2025 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants