rocksdb-cloud: defer restoring skip_cloud_files_in_getchildren

[MrGuin]

Summary by CodeRabbit

• Bug Fixes
  • Enhanced database startup performance by optimizing cloud file operation handling during initialization and configuration steps.

[coderabbitai]

Walkthrough

Modified the timing of flag restoration in RocksDB cloud database startup sequence. The skip_cloud_files_in_getchildren flag now remains enabled throughout configuration steps and is reset only after successful startup completion, rather than immediately after DB::Open.

Changes

Cohort / File(s)	Summary
RocksDB Cloud Startup Optimization store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp	Relocated flag restoration from post-DB::Open to end of startup sequence, keeping skip_cloud_files_in_getchildren enabled during SetOptions/SetDBOptions configuration to prevent expensive cloud file listing operations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A flag that once reset too soon,
Now dances through the startup tune,
Cloud files skip their listing ways,
Till startup bows and ends its days! 🌩️✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The pull request description is entirely missing. The author provided no description, leaving all template checklist items unfulfilled including tests, documentation, issue references, and test suite execution confirmation.	Add a comprehensive pull request description following the template: include test confirmation, document changes, reference related issues/RFCs, and confirm passing test suites.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: deferring the restoration of the skip_cloud_files_in_getchildren flag, which is the primary purpose of the modification.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix_rdb_cld_open

📝 Coding Plan

• Generate coding plan for human review comments

---

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.}

Tip

You can customize the high-level summary generated by CodeRabbit.

Configure the reviews.high_level_summary_instructions setting to provide custom instructions for generating the high-level summary.

[coderabbitai]

🧹 Nitpick comments (1)

store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)

793-796: Consider adding null check after dynamic_cast for consistency.

The existing code at lines 643-649 checks for null after dynamic_cast<CloudFileSystemImpl*>. For consistency and defensive coding, consider adding a similar check here, even though the cast to CloudFileSystem* should always succeed when cloud_fs_ is valid.

🛡️ Optional defensive check

     rocksdb::CloudFileSystem *cfs =
         dynamic_cast<rocksdb::CloudFileSystem *>(cloud_fs_.get());
+    if (cfs == nullptr)
+    {
+        LOG(ERROR) << "Fail to get CloudFileSystem from cloud_fs_";
+        return false;
+    }
     auto &cfs_options_ref = cfs->GetMutableCloudFileSystemOptions();
     cfs_options_ref.skip_cloud_files_in_getchildren = false;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp` around
lines 793 - 796, The dynamic_cast to rocksdb::CloudFileSystem* may return null;
add a null check after assigning cfs (from cloud_fs_) and handle it consistently
(e.g., log an error and return or skip modifying options) before calling
cfs->GetMutableCloudFileSystemOptions(); ensure you reference the same defensive
pattern used earlier around CloudFileSystemImpl* so that cfs is validated before
accessing GetMutableCloudFileSystemOptions() and setting
skip_cloud_files_in_getchildren.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp`:
- Around line 793-796: The dynamic_cast to rocksdb::CloudFileSystem* may return
null; add a null check after assigning cfs (from cloud_fs_) and handle it
consistently (e.g., log an error and return or skip modifying options) before
calling cfs->GetMutableCloudFileSystemOptions(); ensure you reference the same
defensive pattern used earlier around CloudFileSystemImpl* so that cfs is
validated before accessing GetMutableCloudFileSystemOptions() and setting
skip_cloud_files_in_getchildren.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5ca09d0b-dd4b-46a8-90eb-a9a0d1cf9a30

📥 Commits

Reviewing files that changed from the base of the PR and between 1a4729b and 1c8554e.

📒 Files selected for processing (1)

• store_handler/eloq_data_store_service/rocksdb_cloud_data_store.cpp