Fix missing backup_files field when FetchBackup

[githubzilla]

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

• Bug Fixes
  • Ensures snapshot backups are created when using cloud-backed shared storage (Amazon S3, Google Cloud Storage), resolving cases where backups might not trigger under certain build configurations. Maintains existing error handling and fallback behavior, providing more predictable backup behavior across environments.

[coderabbitai]

Walkthrough

Replaces a compile-time guard from ROCKSDB_CLOUD_FS_TYPE to two new feature flags (DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3 and DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_GCS) to control when a backup snapshot is created for shared storage. Runtime logic and error handling paths remain unchanged.

Changes

Cohort / File(s)	Summary
Shared storage snapshot guard update src/store/snapshot_manager.cpp	Replaced preprocessor condition on ROCKSDB_CLOUD_FS_TYPE with new flags: DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_S3 and DATA_STORE_TYPE_ELOQDSS_ROCKSDB_CLOUD_GCS. Snapshot creation for shared storage is compiled only when these flags are defined; existing control flow and error handling unchanged.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Caller
  participant SM as SnapshotManager
  participant SS as SharedStorage (conditional)

  C->>SM: requestSnapshot()
  alt Compiled with S3/GCS flags
    note over SM,SS: New compile-time guard path
    SM->>SS: createBackupSnapshot()
    SS-->>SM: result (success/error)
    SM-->>C: return status
  else Not compiled with flags
    note over SM: Skip shared-storage snapshot path
    SM-->>C: return status (no shared backup)
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I toggled flags with a thump of my paw,
From one big rock to clouds I saw—
S3, GCS, I nibble and choose,
Snapshots packed with carrot-yews.
Compile-time warren, pathways neat,
Backups burrowed, tidy and discreet. 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Title Check	⚠️ Warning	The title “Fix missing backup_files field when FetchBackup” does not describe the actual changes in the pull request, which modify compile-time guards and feature flags for snapshot creation in snapshot_manager.cpp rather than updating a backup_files field or FetchBackup function.	Please update the title to clearly summarize the main change, for example “Replace cloud storage guard macros with new snapshot feature flags,” so it accurately reflects the modifications in snapshot_manager.cpp.
Description Check	⚠️ Warning	The pull request description only repeats the unfilled checklist template and lacks any concrete details about added tests, documentation, issue or RFC references, and test results, so it does not satisfy the repository’s description requirements.	Please complete the checklist by specifying which tests were added or updated, summarizing documentation changes, linking the related issue and any RFC, and confirming that the required test suites pass.
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 (beta)

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

---

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/store/snapshot_manager.cpp (2)

449-468: Critical: Variable shadowing causes backup_files to be empty.

Line 452 declares a local snapshot_files variable that shadows the outer variable declared on line 445. When CreateSnapshotForBackup populates this local variable, the outer snapshot_files remains empty. Subsequently, line 470 calls UpdateBackupTaskStatus with the outer (empty) snapshot_files, resulting in no backup files being stored in the task. This causes GetBackupStatus (lines 677-680) to return an empty file list, directly contradicting the PR title "Fix missing backup_files field when FetchBackup".

Apply this diff to fix the variable 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);

---

483-483: Fix typo: "snpashot" → "snapshot".

Apply this diff:

-        LOG(ERROR) << "Failed to create snpashot for backup";
+        LOG(ERROR) << "Failed to create snapshot for backup";

🧹 Nitpick comments (1)

src/store/snapshot_manager.cpp (1)

443-444: Consider updating the comment for accuracy.

The comment states "For shared storage, need user to call storage to create snapshot", but the code within the feature flag block (lines 451-465) now creates the snapshot automatically. This comment may need updating to reflect the current behavior, or clarify that it applies only when the feature flags are not defined.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f063456 and bcef944.

📒 Files selected for processing (1)

• src/store/snapshot_manager.cpp (1 hunks)