Skip to content

Adapt to submodule rearragement#335

Merged
thweetkomputer merged 2 commits into
mainfrom
adapt_to_eloqstore
Jan 5, 2026
Merged

Adapt to submodule rearragement#335
thweetkomputer merged 2 commits into
mainfrom
adapt_to_eloqstore

Conversation

@thweetkomputer

@thweetkomputer thweetkomputer commented Jan 5, 2026

Copy link
Copy Markdown
Collaborator

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

  • Chores
    • Reorganized internal build configuration and source file structure to use external dependencies more explicitly.
    • Removed INI file format support from the library build.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai

coderabbitai Bot commented Jan 5, 2026

Copy link
Copy Markdown

Walkthrough

A CMake build configuration file was refactored to reorganize external submodule paths under an external/ directory structure, update corresponding include paths, restructure source file lists, and remove INI sources from the library build while simplifying the eloqstore target declaration.

Changes

Cohort / File(s) Summary
CMake Build Configuration
store_handler/eloq_data_store_service/build_eloq_store.cmake
Reorganized submodule paths from top-level to external/ structure (concurrentqueue, abseil); updated include directories to explicitly reference external and internal paths; restructured ELOQ_STORE_SOURCES to use src/ and storage/ organized files; removed INI_SOURCES variable and associated compile definition (INIReader=EloqStorePrivateINIReader); simplified library target to exclude INI sources.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • liunyl
  • MrGuin
  • xiexiaoy

Poem

🐰 The CMake warren is neat,
External paths now complete!
Sources reorganized with care,
Build structures light as air—
Dependencies hopping everywhere! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description contains only the template checklist with no actual details about the changes, missing substantive explanation of what was changed and why. Add a detailed description explaining the submodule path changes, the rationale for the reorganization, and verification steps taken before marking checklist items as complete.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Adapt to submodule rearragement' directly describes the main change: adapting to a submodule reorganization where paths were moved from top-level to external/ subdirectories.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch adapt_to_eloqstore

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Fix all issues with AI Agents 🤖
In @store_handler/eloq_data_store_service/build_eloq_store.cmake:
- Around line 92-124: CMake lists ELOQ_STORE_SOURCES referencing files under
ELOQ_STORE_SOURCE_DIR (the eloqstore module) but that directory and the 31 files
(e.g., async_io_manager.cpp, eloq_store.cpp, data_page.cpp) are missing, so
building with WITH_DATA_STORE=ELOQDSS_ELOQSTORE will fail; fix by either adding
the missing eloqstore source tree (commit files or add the eloqstore git
submodule and ensure ELOQ_STORE_SOURCE_DIR points to it), or modify the CMake to
conditionally skip or remove ELOQ_STORE_SOURCES when WITH_DATA_STORE !=
ELOQDSS_ELOQSTORE (or default the option to disabled), updating references to
ELOQ_STORE_SOURCES and ELOQ_STORE_SOURCE_DIR accordingly.
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 840b9e7 and ad681e2.

📒 Files selected for processing (1)
  • store_handler/eloq_data_store_service/build_eloq_store.cmake
🔇 Additional comments (3)
store_handler/eloq_data_store_service/build_eloq_store.cmake (3)

84-90: LGTM!

The include directories are correctly updated to reflect the new external/, include/, and root directory structure. This aligns well with the submodule reorganization.


75-82: Submodule path updates are correct—no stale references found.

The search confirms that all references to concurrentqueue and abseil within build_eloq_store.cmake correctly use the new external/ paths. Other modules maintain their own separate submodule structures and do not require updates.


125-125: Original review comment is incorrect.

The concern about removing ${INI_SOURCES} from the eloqstore library is invalid. INI functionality was never part of this library. The eloqstore library is built exclusively from ${ELOQ_STORE_SOURCES} and contains no INI-related dependencies. INIReader.cpp is compiled separately as part of other targets (tx_service, log_service, eloq_data_store_service), not as part of eloqstore. Configuration classes that require INI parsing (RocksDBConfig, EloqStoreConfig) receive INIReader instances as constructor parameters from their own respective build contexts, not from the eloqstore library.

Likely an incorrect or invalid review comment.

Comment thread store_handler/eloq_data_store_service/build_eloq_store.cmake
@thweetkomputer thweetkomputer merged commit bc6852e into main Jan 5, 2026
4 checks passed
@thweetkomputer thweetkomputer deleted the adapt_to_eloqstore branch January 5, 2026 06:30
@coderabbitai coderabbitai Bot mentioned this pull request Jan 8, 2026
5 tasks
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