Open
Conversation
In cloud/shared-storage mode CreateClusterBackup does not supply a dest_path because the snapshot is written directly to object storage (no rsync transfer is needed). The assertion fired for every cloud backup request, causing a SIGABRT crash. Replace with a comment explaining the expected behaviour.
"main" is the canonical production branch name in eloqstore; using "development" as the default was misleading.
Add INIReader support for eloq_dss_branch_name so it can be set in the [store] section of the config file, consistent with other store flags. Command-line flag still takes precedence over the config file value.
Apply the same CheckCommandLineFlagIsDefault + INIReader pattern as storage_init.cpp so eloq_dss_branch_name can be set via the [local] section of the DSS config file. Command-line flag still takes precedence.
…isting completed backups
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR updates the default EloqDataStore branch from "development" to "main" and implements branch-aware snapshot and backup creation. It adds branch_name_ configuration to EloqStoreConfig and EloqStoreDataStore, applies consistent branch name resolution across storage initialization, enhances CreateSnapshotForBackup to support branching operations, and makes snapshot manager backup creation idempotent. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
store_handler/eloq_data_store_service/main.cpp (1)
73-73:⚠️ Potential issue | 🟠 MajorInconsistent default value: should be
"main"per PR objective.This file defines
FLAGS_eloq_dss_branch_namewith default"development", butcore/src/storage_init.cpp(line 87) defines the same flag with default"main". The PR commit message states the intent is to change the default from"development"to"main".This inconsistency means the standalone
eloq_dssbinary will use"development"while the integrated tx_service will use"main"by default, leading to potential confusion and different behaviors across deployments.🐛 Proposed fix
-DEFINE_string(eloq_dss_branch_name, "development", "Data store branch name."); +DEFINE_string(eloq_dss_branch_name, "main", "Data store branch name.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store_handler/eloq_data_store_service/main.cpp` at line 73, Change the default for FLAGS_eloq_dss_branch_name in the DEFINE_string call (DEFINE_string(eloq_dss_branch_name, "development", ...)) to "main" so it matches the default used in core's storage_init (and the PR intent); update the second argument of DEFINE_string for eloq_dss_branch_name from "development" to "main" to keep behavior consistent across binaries.
🧹 Nitpick comments (1)
store_handler/eloq_data_store_service/eloq_store_data_store.cpp (1)
28-31: Remove duplicate#include <memory>.
<memory>is included twice: once at line 26 and again at line 30.🧹 Proposed fix
`#include` <cassert> `#include` <filesystem> -#include <memory> `#include` "common.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store_handler/eloq_data_store_service/eloq_store_data_store.cpp` around lines 28 - 31, The file has a duplicate include of the same header (<memory>) in the include block; remove the redundant "#include <memory>" so only a single include remains (locate and edit the include block containing <cassert>, <filesystem>, and the duplicated <memory> lines and delete the extra <memory> entry).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@store_handler/eloq_data_store_service/main.cpp`:
- Line 73: Change the default for FLAGS_eloq_dss_branch_name in the
DEFINE_string call (DEFINE_string(eloq_dss_branch_name, "development", ...)) to
"main" so it matches the default used in core's storage_init (and the PR
intent); update the second argument of DEFINE_string for eloq_dss_branch_name
from "development" to "main" to keep behavior consistent across binaries.
---
Nitpick comments:
In `@store_handler/eloq_data_store_service/eloq_store_data_store.cpp`:
- Around line 28-31: The file has a duplicate include of the same header
(<memory>) in the include block; remove the redundant "#include <memory>" so
only a single include remains (locate and edit the include block containing
<cassert>, <filesystem>, and the duplicated <memory> lines and delete the extra
<memory> entry).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3d3d405-c84b-44f7-988b-2fb78905b8dc
📒 Files selected for processing (8)
core/src/storage_init.cppstore_handler/eloq_data_store_service/eloq_store_config.hstore_handler/eloq_data_store_service/eloq_store_data_store.cppstore_handler/eloq_data_store_service/eloq_store_data_store.hstore_handler/eloq_data_store_service/eloqstorestore_handler/eloq_data_store_service/main.cpptx_service/src/remote/cc_node_service.cpptx_service/src/store/snapshot_manager.cpp
Fix manifest deletion bug in file_gc
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@store_handler/eloq_data_store_service/eloq_store_config.cpp`:
- Around line 133-135: The change temporarily setting the flag
DEFINE_uint32(eloq_store_pages_per_file_shift, 2, ...) was committed with a
"temp" message and should not be merged to main; either revert the value to the
production default (e.g., restore the previous shift such as 11) and remove the
"temp" commit, or move this change to a feature/testing branch and update the
commit/PR message to describe the experiment; locate the DEFINE_uint32 call for
eloq_store_pages_per_file_shift in eloq_store_config.cpp and either restore the
original value or create a feature branch and update commit metadata before
merging.
In `@store_handler/eloq_data_store_service/main.cpp`:
- Around line 334-338: The branch_name assignment uses config_reader.GetString
with the wrong section ("local"); update the call so
eloq_store_config.branch_name_ falls back to config_reader.GetString("store",
"eloq_dss_branch_name", FLAGS_eloq_dss_branch_name) when
CheckCommandLineFlagIsDefault("eloq_dss_branch_name") is true, keeping the
existing use of FLAGS_eloq_dss_branch_name and the
CheckCommandLineFlagIsDefault/FLAGS_eloq_dss_branch_name logic intact.
- Around line 306-310: The code in main.cpp reads eloq_dss_branch_name from the
"local" section causing a mismatch with storage_init.cpp; update the config
lookup in the rocksdb_cloud_config.branch_name_ assignment (the call site using
CheckCommandLineFlagIsDefault, FLAGS_eloq_dss_branch_name and
config_reader.GetString) and the other similar occurrence that currently uses
"local" so both use the "store" section instead to match storage_init.cpp and
avoid silent misconfiguration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a6d89b0-0a0c-4acc-b03b-5f6d47e697aa
📒 Files selected for processing (3)
store_handler/eloq_data_store_service/eloq_store_config.cppstore_handler/eloq_data_store_service/eloqstorestore_handler/eloq_data_store_service/main.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- store_handler/eloq_data_store_service/eloqstore
…hRequest Update call site in eloq_store_data_store.cpp to match the simplified SetArgs signature and bump the eloqstore submodule pointer.
… 'local' The branch_name config lookup in main.cpp used the 'local' INI section, while storage_init.cpp uses the 'store' section. This mismatch caused silent misconfiguration when users set eloq_dss_branch_name under the [store] section. Updated both occurrences (rocksdb_cloud_config and eloq_store_config) to use 'store' for consistency.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Here are some reminders before you submit the pull request
fixes eloqdb/tx_service#issue_id./mtr --suite=mono_main,mono_multi,mono_basicSummary by CodeRabbit
Configuration
New Features
Bug Fixes