Skip to content

add eloqstore prewarm info#144

Merged
thweetkomputer merged 2 commits intoeloqdata:mainfrom
eatbreads:add_eloq_build
Nov 18, 2025
Merged

add eloqstore prewarm info#144
thweetkomputer merged 2 commits intoeloqdata:mainfrom
eatbreads:add_eloq_build

Conversation

@eatbreads
Copy link
Contributor

@eatbreads eatbreads commented Nov 14, 2025

Summary by CodeRabbit

  • New Features

    • Added a startup option to optionally prewarm the cloud cache (off by default).
  • Chores

    • Build configuration updated to include components that support the new prewarm capability.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 14, 2025

Walkthrough

Adds a new source file (prewarm_task.cpp) to the eloqstore build and introduces a new boolean configuration flag (eloq_store_prewarm_cloud_cache) with parsing logic that respects command-line flag precedence over INI settings; exposes the value via eloqstore_configs_.prewarm_cloud_cache.

Changes

Cohort / File(s) Change Summary
Build configuration update
eloq_data_store_service/build_eloq_store.cmake
Adds ${ELOQ_STORE_SOURCE_DIR}/prewarm_task.cpp to the ELOQ_STORE_SOURCES list for compilation.
Config + flag addition
eloq_data_store_service/eloq_store_config.cpp
Adds DEFINE_bool(eloq_store_prewarm_cloud_cache, false, ...); reads the flag/INI setting with flag precedence and initializes eloqstore_configs_.prewarm_cloud_cache.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Startup as Service Startup
  participant Flags as Command-line Flags
  participant INI as INI Config
  participant Config as EloqStoreConfig

  rect rgba(100,150,240,0.06)
  Note over Flags,INI: Configuration resolution for prewarm_cloud_cache
  end

  Startup->>Flags: check FLAGS_eloq_store_prewarm_cloud_cache
  alt Flag explicitly set
    Flags-->>Config: use flag value
  else Flag not set
    Startup->>INI: read store.eloq_store_prewarm_cloud_cache (optional)
    INI-->>Config: provide INI value or fallback to FLAGS_eloq_store_prewarm_cloud_cache
  end
  Config-->>Startup: eloqstore_configs_.prewarm_cloud_cache initialized
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify existence and correct path of prewarm_task.cpp.
  • Review new flag declaration for naming/visibility and help text.
  • Inspect parsing logic in eloq_store_config.cpp for correct precedence and defaulting.
  • Check for any missing includes or build implications of adding the new source file.

Possibly related PRs

Poem

🐰 A new file hops into the tree,
A flag decides what cache will be,
Startup peeks at flag then file,
Prewarm dreams in CMake style. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
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.
Title check ❓ Inconclusive The title 'add eloqstore prewarm info' is vague and generic. While it references 'eloqstore' and 'prewarm', the term 'info' is non-descriptive and doesn't clearly convey what is actually being added or changed. Consider a more specific title that describes the actual implementation, such as 'Add eloqstore cloud cache prewarm configuration flag' or 'Introduce prewarm_cloud_cache configuration option for eloqstore'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
eloq_data_store_service/eloq_store_config.cpp (1)

108-120: Consider extracting duplicated helper to a common header.

CheckCommandLineFlagIsDefault is duplicated in at least three locations: this file, main.cpp, and rocksdb_config.cpp. Extract this helper into a shared utility header (e.g., gflags_util.h) to reduce duplication and improve maintainability.

Example refactor to create a shared header:

// eloq_data_store_service/gflags_util.h
#pragma once

#include <cassert>
#include "gflags/gflags.h"

namespace EloqDS {

inline bool CheckCommandLineFlagIsDefault(const char *name) {
    gflags::CommandLineFlagInfo flag_info;
    bool flag_found = gflags::GetCommandLineFlagInfo(name, &flag_info);
    assert(flag_found);
    (void) flag_found;
    return flag_info.is_default;
}

}  // namespace EloqDS

Then replace all three duplicated definitions with #include "gflags_util.h".

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 75729f6 and dfb8619.

📒 Files selected for processing (1)
  • eloq_data_store_service/eloq_store_config.cpp (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
eloq_data_store_service/eloq_store_config.cpp (3)
eloq_data_store_service/rocksdb_config.cpp (2)
  • CheckCommandLineFlagIsDefault (300-312)
  • CheckCommandLineFlagIsDefault (300-300)
rocksdb_config.cpp (2)
  • CheckCommandLineFlagIsDefault (298-310)
  • CheckCommandLineFlagIsDefault (298-298)
eloq_data_store_service/main.cpp (2)
  • CheckCommandLineFlagIsDefault (88-100)
  • CheckCommandLineFlagIsDefault (88-88)
🔇 Additional comments (2)
eloq_data_store_service/eloq_store_config.cpp (2)

93-95: LGTM! Flag definition follows established patterns.

The new boolean flag follows the existing naming conventions, has a safe default value (false), and includes a clear description.


376-381: Dead code: prewarm_cloud_cache flag is parsed but never used.

The code under review correctly parses the flag following established patterns. However, verification shows prewarm_cloud_cache is never accessed or used anywhere in the codebase—it's configured but remains unused. The original review's concern about missing cloud storage guards is moot since the flag is never read.

The cloud storage path is properly guarded where actually used (eloq_store_data_store.cpp:86). Either implement the prewarming functionality that uses this flag, or remove the unused configuration.

Likely an incorrect or invalid review comment.

@thweetkomputer thweetkomputer merged commit 1c00129 into eloqdata:main Nov 18, 2025
1 check passed
@coderabbitai coderabbitai bot mentioned this pull request Nov 28, 2025
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