Avoid forcing checkpoint flush when data sync memory quota is full

[thweetkomputer]

Here are some reminders before you submit the pull request

• Add tests for the change
• Document changes
• Reference the link of issue using fixes eloqdb/tx_service#issue_id
• Reference the link of RFC if exists
• Pass ./mtr --suite=mono_main,mono_multi,mono_basic

Summary by CodeRabbit

• Refactor

  • Simplified memory allocation behavior to rely on existing synchronization mechanisms rather than proactive flushing.

• Style

  • Applied minor formatting adjustments to improve code consistency.

[coderabbitai]

Walkthrough

Removed a proactive memory flush call in DataSyncMemoryController::AllocateFlushDataMemQuota, shifting memory management to rely solely on wait loops rather than immediate flushing when memory shortage occurs. Also applied minor whitespace formatting in snapshot manager code.

Changes

Cohort / File(s)	Summary
Memory Management Logic include/cc/local_cc_shards.h	Removed pre-wait flush handling block that proactively called FlushCurrentFlushBuffer on memory shortage; control flow now relies on existing wait loop
Formatting & Whitespace src/store/snapshot_manager.cpp	Minor whitespace and line-ending adjustment within cloud storage backend preprocessor conditional; no functional changes

Sequence Diagram(s)

sequenceDiagram
    participant App as Application
    participant Controller as DataSyncMemoryController
    participant Shards as LocalCcShards
    participant Wait as Wait Loop

    rect rgba(255, 220, 150, 0.3)
        Note over Controller: Old Behavior
        App->>Controller: AllocateFlushDataMemQuota()
        alt Memory Available
            Controller-->>App: Success
        else Memory Shortage
            Controller->>Shards: FlushCurrentFlushBuffer()
            Shards-->>Controller: Flush Complete
            Controller->>Wait: Wait for Memory
            Wait-->>Controller: Memory Available
            Controller-->>App: Success
        end
    end

    rect rgba(180, 220, 150, 0.3)
        Note over Controller: New Behavior
        App->>Controller: AllocateFlushDataMemQuota()
        alt Memory Available
            Controller-->>App: Success
        else Memory Shortage
            Controller->>Wait: Wait for Memory
            Wait-->>Controller: Memory Available
            Controller-->>App: Success
        end
    end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested reviewers

• liunyl
• lzxddz
• MrGuin

Poem

🐰 A memory hop, a flush no more,
We wait instead, and still explore,
Less eager flushing, calmer flow,
Watch that quota gently grow! 💾✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The pull request description consists only of the template checklist with all items unchecked and no actual content describing the changes. While the template structure is present, there is no substantive description of what the PR does, why the changes were made, what problem is being solved, or any implementation details. This results in a largely incomplete description that fails to communicate the purpose and scope of the modifications beyond what can be inferred from the file summaries.	The author should fill in the description with a clear explanation of the changes, including what was modified, why this change was necessary, and what impact it has. Additionally, the unchecked items suggest that the required tasks (adding tests, documenting changes, referencing issues, and running the test suite) have not yet been completed or verified. The author should update the description with substantive content and check off items as they are completed.
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.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The pull request title "Avoid forcing checkpoint flush when data sync memory quota is full" is directly related to the primary change in the PR. The main modification in include/cc/local_cc_shards.h removes a proactive flush call in DataSyncMemoryController::AllocateFlushDataMemQuota when memory is insufficient, which is exactly what the title describes. The title is clear, specific, concise, and avoids vague terminology or noise, making it easy for reviewers to understand the core change at a glance.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch mod_ckpt_zc

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/store/snapshot_manager.cpp (1)

447-476: Consider eliminating variable shadowing for snapshot_files.

At line 452, snapshot_files is redeclared inside the preprocessor conditional, shadowing the outer declaration from line 445. While this appears to work correctly (the inner one is used in the cloud storage path, outer one in the non-cloud path), it's confusing and error-prone—future maintainers might mistakenly assume the outer vector is being populated throughout this block.

Recommend reusing the outer snapshot_files without redeclaring it at line 452 to improve clarity.

Consider applying this diff to eliminate shadowing:

#if (defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3) || \
     defined(DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_GCS))
         // For shared storage with cloud filesystem enabled, create snapshot
-        std::vector<std::string> snapshot_files;
         bool res = store_hd_->CreateSnapshotForBackup(
             backup_name, snapshot_files, last_ckpt_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 b823488 and aec150c.

📒 Files selected for processing (2)

• include/cc/local_cc_shards.h (0 hunks)
• src/store/snapshot_manager.cpp (1 hunks)

💤 Files with no reviewable changes (1)

• include/cc/local_cc_shards.h

🔇 Additional comments (1)

src/store/snapshot_manager.cpp (1)

449-450: Preprocessor formatting appears appropriate.

The line continuation and indentation for the cloud storage backend check looks clean. Assuming this is the only formatting change in this section, it aligns with the PR objective of a minor whitespace adjustment.