feat(core)!: Replace YAML config with CLI args and env vars for metadata DB (resolves #1146).#1148
Merged
Conversation
Contributor
WalkthroughReplaces YAML-based global metadata DB configuration with CLI options and environment-variable-backed credentials; removes yaml-cpp; adds a create_global_metadata_db factory; updates CLP/CLG/CLP-S command-line handling, tests, init-db script, docs, CMake, lint, and job-orchestration integration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLP/CLG CLI
participant GDC as GlobalMetadataDBConfig
participant Env as Environment
participant Factory as create_global_metadata_db
participant DB as GlobalMetadataDB
User->>CLI: Invoke with --db-type/--db-host/--db-port/--db-name/--db-table-prefix
CLI->>GDC: Construct via options_description
CLI->>Env: Read CLP_DB_USER / CLP_DB_PASS if DB type != sqlite
CLI->>GDC: read_credentials_from_env_if_needed()
CLI->>GDC: validate()
CLI->>Factory: create_global_metadata_db(GDC, archives_dir)
Factory->>DB: Instantiate SQLite or MySQL (or return nullptr)
DB-->>CLI: Instance or null
CLI->>DB: open() (if instance)
sequenceDiagram
participant Orchestrator as compression_task.py
participant CLP as clp subprocess
participant Indexer as clp_s indexer subprocess
participant Env as Process Env
Orchestrator->>Orchestrator: Build DB CLI args and env (CLP_DB_USER/PASS)
Orchestrator->>CLP: Launch CLP with args + env
Orchestrator->>Indexer: Launch Indexer with args + env (table_prefix included)
CLP->>Env: Process uses provided env vars for DB credentials
Indexer->>Env: Process uses provided env vars for DB credentials
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the "Integrations" page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}⚙️ CodeRabbit Configuration File
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2024-12-05T16:32:21.507ZApplied to files:
📚 Learning: 2025-07-23T09:54:45.185ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
components/core/CMakeLists.txt(1 hunks)components/core/config/metadata-db.yml(0 hunks)components/core/src/clp/GlobalMetadataDBConfig.cpp(1 hunks)components/core/src/clp/GlobalMetadataDBConfig.hpp(3 hunks)components/core/src/clp/clg/CMakeLists.txt(0 hunks)components/core/src/clp/clg/CommandLineArguments.cpp(2 hunks)components/core/src/clp/clp/CMakeLists.txt(0 hunks)components/core/src/clp/clp/CommandLineArguments.cpp(2 hunks)components/core/src/clp_s/indexer/CMakeLists.txt(0 hunks)components/core/src/clp_s/indexer/CommandLineArguments.cpp(2 hunks)components/core/tests/test-GlobalMetadataDBConfig.cpp(1 hunks)components/core/tools/scripts/db/init-db.py(3 hunks)components/job-orchestration/job_orchestration/executor/compress/compression_task.py(5 hunks)docs/src/user-guide/core-unstructured/clp.md(1 hunks)
💤 Files with no reviewable changes (4)
- components/core/src/clp/clg/CMakeLists.txt
- components/core/src/clp_s/indexer/CMakeLists.txt
- components/core/src/clp/clp/CMakeLists.txt
- components/core/config/metadata-db.yml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/tests/test-GlobalMetadataDBConfig.cppcomponents/core/src/clp_s/indexer/CommandLineArguments.cppcomponents/core/src/clp/clg/CommandLineArguments.cppcomponents/core/src/clp/GlobalMetadataDBConfig.hppcomponents/core/src/clp/clp/CommandLineArguments.cppcomponents/core/src/clp/GlobalMetadataDBConfig.cpp
🧠 Learnings (32)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 Learning: in 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floa...
Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-end_to_end.cpp:59-65
Timestamp: 2024-11-19T17:30:04.970Z
Learning: In 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floats' test, files and directories are intentionally removed at the beginning of the test to ensure that any existing content doesn't influence the test results.
Applied to files:
components/core/CMakeLists.txtcomponents/core/tests/test-GlobalMetadataDBConfig.cpp
📚 Learning: in the clp project, when reviewing cmakelists.txt changes that introduce new compression library dep...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
components/core/CMakeLists.txtdocs/src/user-guide/core-unstructured/clp.mdcomponents/job-orchestration/job_orchestration/executor/compress/compression_task.py
📚 Learning: in the file `components/core/tests/test-ffi_keyvaluepairlogevent.cpp`, including `
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
Applied to files:
components/core/CMakeLists.txt
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:14-14
Timestamp: 2024-10-14T03:42:10.355Z
Learning: In the file `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, including `<json/single_include/nlohmann/json.hpp>` is consistent with the project's coding standards.
components/core/CMakeLists.txt📚 Learning: in the clp project, antlr code generation at build time is being removed by another pr. when reviewi...
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Applied to files:
components/core/CMakeLists.txtcomponents/core/src/clp/clp/CommandLineArguments.cpp
📚 Learning: in the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msg...
Learnt from: LinZhihao-723
PR: y-scope/clp#570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
Applied to files:
components/core/CMakeLists.txtcomponents/core/tests/test-GlobalMetadataDBConfig.cpp
📚 Learning: in cmakelists.txt files, anlowee prefers to explicitly list source files one by one rather than usin...
Learnt from: anlowee
PR: y-scope/clp#925
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-19
Timestamp: 2025-05-26T18:39:51.727Z
Learning: In CMakeLists.txt files, anlowee prefers to explicitly list source files one by one rather than using file(GLOB ...) or file(GLOB_RECURSE ...) to automatically include files, maintaining consistency with other CMake files in the project.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: the team plans to systematically improve include paths by adding `${project_source_dir}/src` to cmak...
Learnt from: LinZhihao-723
PR: y-scope/clp#873
File: components/core/src/clp/ffi/ir_stream/search/test/utils.hpp:15-18
Timestamp: 2025-05-02T22:11:55.711Z
Learning: The team plans to systematically improve include paths by adding `${PROJECT_SOURCE_DIR}/src` to CMake targets and using direct include paths instead of deep relative ones in a future PR.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: in `components/core/tests/test-ffi_keyvaluepairlogevent.cpp`, helper functions like `assert_kv_pair_...
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:228-236
Timestamp: 2024-10-14T03:43:40.364Z
Learning: In `components/core/tests/test-ffi_KeyValuePairLogEvent.cpp`, helper functions like `assert_kv_pair_log_event_creation_failure` return booleans to the caller, and the caller asserts using `REQUIRE` to provide clearer failure information.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: the function `assert_kv_pair_log_event_creation_failure` is correctly placed within the anonymous na...
Learnt from: LinZhihao-723
PR: y-scope/clp#558
File: components/core/tests/test-ffi_KeyValuePairLogEvent.cpp:85-103
Timestamp: 2024-10-14T03:42:53.145Z
Learning: The function `assert_kv_pair_log_event_creation_failure` is correctly placed within the anonymous namespace in `test-ffi_KeyValuePairLogEvent.cpp`.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: in the unit test case `ffi_ir_stream_serialize_schema_tree_node_id` in `test-ir_encoding_methods.cpp...
Learnt from: LinZhihao-723
PR: y-scope/clp#557
File: components/core/tests/test-ir_encoding_methods.cpp:1216-1286
Timestamp: 2024-10-13T09:27:43.408Z
Learning: In the unit test case `ffi_ir_stream_serialize_schema_tree_node_id` in `test-ir_encoding_methods.cpp`, suppressing the `readability-function-cognitive-complexity` warning is acceptable due to the expansion of Catch2 macros in C++ tests, and such test cases may not have readability issues.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: in the clp project, manually setting ldflags for library paths can cause runtime errors because it i...
Learnt from: sitaowang1998
PR: y-scope/clp#1044
File: .github/workflows/clp-core-build-macos.yaml:0-0
Timestamp: 2025-06-27T01:49:15.724Z
Learning: In the clp project, manually setting LDFLAGS for library paths can cause runtime errors because it interferes with CMake's built-in library discovery and linking mechanisms. CMake can identify and correctly link to libraries on its own without requiring manual LDFLAGS configuration.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: in clp's custom cmake find modules (like findbzip2.cmake), when building with dynamic libraries, the...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1124
File: components/core/cmake/Modules/FindBZip2.cmake:60-60
Timestamp: 2025-07-24T21:56:05.806Z
Learning: In CLP's custom CMake Find modules (like FindBZip2.cmake), when building with dynamic libraries, the code intentionally uses `bzip2_PKGCONF_STATIC_LIBRARIES` as input to `FindDynamicLibraryDependencies`. This is a deliberate fallback mechanism to ensure all library dependencies are resolved even in dynamic builds, using the static library list from pkg-config as a source of dependency information.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: in the clp project, c++20 is used, allowing the utilization of c++20 features such as class template...
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:37-38
Timestamp: 2024-11-26T19:12:43.982Z
Learning: In the CLP project, C++20 is used, allowing the utilization of C++20 features such as class template argument deduction (CTAD) with `std::array`.
Applied to files:
components/core/CMakeLists.txtcomponents/core/src/clp/GlobalMetadataDBConfig.hpp
📚 Learning: in the clp codebase, standard headers like `` and `` in alphabetical order (as seen...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/src/glt/SQLitePreparedStatement.hpp:4-4
Timestamp: 2025-07-01T14:49:07.333Z
Learning: In the CLP codebase, standard headers like `<cstdint>` and `<string>` in alphabetical order (as seen in components/core/src/glt/SQLitePreparedStatement.hpp) are correctly ordered and do not need adjustment.
Applied to files:
components/core/CMakeLists.txtcomponents/core/src/clp/GlobalMetadataDBConfig.hpp
📚 Learning: in `components/core/tests/test-clp_s-end_to_end.cpp`, the success of `constructor.store()` is verifi...
Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-clp_s-end_to_end.cpp:109-110
Timestamp: 2024-11-29T22:50:17.206Z
Learning: In `components/core/tests/test-clp_s-end_to_end.cpp`, the success of `constructor.store()` is verified through `REQUIRE` statements and subsequent comparisons.
Applied to files:
components/core/tests/test-GlobalMetadataDBConfig.cpp
📚 Learning: in 'components/core/tests/test-clp_s-end_to_end.cpp', using `wifexited` and `wexitstatus` with the r...
Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-clp_s-end_to_end.cpp:141-142
Timestamp: 2024-11-29T22:51:00.355Z
Learning: In 'components/core/tests/test-clp_s-end_to_end.cpp', using `WIFEXITED` and `WEXITSTATUS` with the return value of `std::system` is acceptable for checking the exit status of system commands, as per the C++ documentation.
Applied to files:
components/core/tests/test-GlobalMetadataDBConfig.cpp
📚 Learning: member variables in c++ classes should be explicitly initialized in the constructor to prevent undef...
Learnt from: haiqi96
PR: y-scope/clp#646
File: components/core/src/clp/streaming_archive/writer/Archive.hpp:354-354
Timestamp: 2025-01-14T16:06:54.692Z
Learning: Member variables in C++ classes should be explicitly initialized in the constructor to prevent undefined behavior, as demonstrated in the Archive class where `m_use_single_file_archive` is initialized to `false`.
Applied to files:
components/core/src/clp_s/indexer/CommandLineArguments.cpp
📚 Learning: in `clp_package_utils/scripts/native/del_archives.py`, when deleting archives, the `archive` variabl...
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:104-110
Timestamp: 2024-11-15T16:21:52.122Z
Learning: In `clp_package_utils/scripts/native/del_archives.py`, when deleting archives, the `archive` variable retrieved from the database is controlled and is always a single string without path components. Therefore, it's acceptable to skip additional validation checks for directory traversal in this context.
Applied to files:
components/core/src/clp_s/indexer/CommandLineArguments.cppcomponents/job-orchestration/job_orchestration/executor/compress/compression_task.py
📚 Learning: in `components/core/src/clp_s/jsonparser.cpp`, validation and exception throwing are unnecessary in ...
Learnt from: gibber9809
PR: y-scope/clp#630
File: components/core/src/clp_s/JsonParser.cpp:702-703
Timestamp: 2024-12-10T16:03:13.322Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, validation and exception throwing are unnecessary in the `get_archive_node_id` method when processing nodes, and should not be added.
Applied to files:
components/core/src/clp_s/indexer/CommandLineArguments.cppcomponents/core/src/clp/GlobalMetadataDBConfig.cpp
📚 Learning: the clp project team prefers to use video content to demonstrate detailed procedural steps (like tar...
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in clp-s documentation, technical abbreviations like "mpt" (merged parse tree) should be defined at ...
Learnt from: quinntaylormitchell
PR: y-scope/clp#961
File: docs/src/dev-guide/design-clp-structured/single-file-archive-format.md:216-219
Timestamp: 2025-06-18T14:35:20.485Z
Learning: In clp-s documentation, technical abbreviations like "MPT" (Merged Parse Tree) should be defined at first use to improve reader clarity and comprehension.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: the clp project team has decided to refrain from using include directives in their documentation at ...
Learnt from: quinntaylormitchell
PR: y-scope/clp#1069
File: docs/src/user-guide/quick-start/clp-json.md:135-152
Timestamp: 2025-07-05T03:38:16.779Z
Learning: The CLP project team has decided to refrain from using include directives in their documentation at present, preferring to maintain duplicated content rather than using shared includes or partials for de-duplication.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in the clp-package compression flow, path validation and error handling is performed at the schedule...
Learnt from: haiqi96
PR: y-scope/clp#651
File: components/clp-package-utils/clp_package_utils/scripts/compress.py:0-0
Timestamp: 2025-01-16T16:58:43.190Z
Learning: In the clp-package compression flow, path validation and error handling is performed at the scheduler level rather than in the compress.py script to maintain simplicity and avoid code duplication.
Applied to files:
docs/src/user-guide/core-unstructured/clp.mdcomponents/job-orchestration/job_orchestration/executor/compress/compression_task.py
📚 Learning: in clp installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistenc...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in the compression scheduler, the team prefers initializing in-memory caches from the database at st...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#831
File: components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py:0-0
Timestamp: 2025-04-17T16:55:06.658Z
Learning: In the compression scheduler, the team prefers initializing in-memory caches from the database at startup rather than performing repeated database queries for efficiency reasons. This approach maintains both performance and reliability across process restarts.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in `components/core/src/clp_s/schematree.hpp`, the `schemanode` class uses `std::unique_ptr ...
Learnt from: gibber9809
PR: y-scope/clp#584
File: components/core/src/clp_s/SchemaTree.hpp:91-94
Timestamp: 2024-11-12T18:47:03.828Z
Learning: In `components/core/src/clp_s/SchemaTree.hpp`, the `SchemaNode` class uses `std::unique_ptr<char[]> m_key_buf` and `std::string_view m_key_name` to ensure that references to `m_key_name` remain valid even after `SchemaNode` is move-constructed.
Applied to files:
components/core/src/clp/GlobalMetadataDBConfig.hpp
📚 Learning: in the clp codebase, `table_prefix` is a trusted configuration value and can be safely embedded into...
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:90-96
Timestamp: 2024-11-15T16:22:33.635Z
Learning: In the CLP codebase, `table_prefix` is a trusted configuration value and can be safely embedded into SQL queries.
Applied to files:
components/core/tools/scripts/db/init-db.pycomponents/job-orchestration/job_orchestration/executor/compress/compression_task.py
📚 Learning: in `components/core/src/clp/bufferedfilereader.cpp`, refactoring the nested error handling condition...
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/BufferedFileReader.cpp:96-106
Timestamp: 2024-10-24T14:45:26.265Z
Learning: In `components/core/src/clp/BufferedFileReader.cpp`, refactoring the nested error handling conditions may not apply due to the specific logic in the original code.
Applied to files:
components/core/src/clp/clp/CommandLineArguments.cppcomponents/core/src/clp/GlobalMetadataDBConfig.cpp
📚 Learning: in `fs_compression_task.py`, when refactoring the archive processing loop in python, the `src_archiv...
Learnt from: haiqi96
PR: y-scope/clp#634
File: components/job-orchestration/job_orchestration/executor/compress/fs_compression_task.py:0-0
Timestamp: 2024-12-16T21:49:06.483Z
Learning: In `fs_compression_task.py`, when refactoring the archive processing loop in Python, the `src_archive_file.unlink()` operation should remain outside of the `process_archive` function.
Applied to files:
components/job-orchestration/job_orchestration/executor/compress/compression_task.py
📚 Learning: the column metadata table (created by `_create_column_metadata_table`) is only needed for dataset-sp...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#868
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py:141-144
Timestamp: 2025-05-05T16:32:55.163Z
Learning: The column metadata table (created by `_create_column_metadata_table`) is only needed for dataset-specific workflows in `CLP_S` and is obsolete for non-dataset workflows.
Applied to files:
components/job-orchestration/job_orchestration/executor/compress/compression_task.py
📚 Learning: in the clp codebase, the validate_and_cache_dataset function in components/clp-py-utils/clp_py_utils...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1036
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py:204-211
Timestamp: 2025-07-03T12:58:18.407Z
Learning: In the CLP codebase, the validate_and_cache_dataset function in components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py uses in-place updates of the existing_datasets set parameter rather than returning a new set, as preferred by the development team.
Applied to files:
components/job-orchestration/job_orchestration/executor/compress/compression_task.py
🧬 Code Graph Analysis (2)
components/core/src/clp_s/indexer/CommandLineArguments.cpp (2)
components/core/src/clp/GlobalMetadataDBConfig.cpp (2)
add_command_line_options(13-32)add_command_line_options(13-15)components/core/src/clp_s/indexer/CommandLineArguments.hpp (2)
m_program_name(29-29)m_metadata_db_config(35-37)
components/core/src/clp/GlobalMetadataDBConfig.cpp (1)
components/core/src/clp/GlobalMetadataDBConfig.hpp (2)
options_description(26-28)parsed_options(38-39)
🪛 LanguageTool
docs/src/user-guide/core-unstructured/clp.md
[style] ~117-~117: Consider using “who” when you are referring to a person instead of an object.
Context: ...tions for your platform * Create a user that has privileges to create databases, cre...
(THAT_WHO)
[uncategorized] ~119-~119: Possible missing comma found.
Context: ... records. * Install the MariaDB Python package pip3 install mariadb * This is nece...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
docs/src/user-guide/core-unstructured/clp.md
109-109: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
122-122: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
131-131: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
🪛 GitHub Actions: clp-lint
components/job-orchestration/job_orchestration/executor/compress/compression_task.py
[error] 180-210: Black formatting check failed. File would be reformatted. Run 'black --write' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
🔇 Additional comments (27)
components/core/CMakeLists.txt (1)
719-719: LGTM! New test file added for refactored configuration.The addition of
tests/test-GlobalMetadataDBConfig.cppproperly supports the new command-line and environment variable-based configuration approach, replacing the previous YAML-based configuration.components/core/tools/scripts/db/init-db.py (3)
4-5: LGTM! Added necessary import for environment variable access.The
osimport is required for reading database credentials from environment variables, replacing the previous YAML configuration approach.
22-40: LGTM! Well-implemented command-line argument parsing and credential handling.The script correctly:
- Defines command-line arguments for database connection parameters
- Reads credentials from environment variables
CLP_DB_USERandCLP_DB_PASS- Provides clear error messages for missing environment variables
- Maintains proper separation of connection parameters and credentials
51-51: LGTM! Proper SQL identifier quoting implemented.The database and table names are correctly quoted with backticks, which is the proper way to handle SQL identifiers in MySQL, especially when they might contain special characters or reserved words.
Also applies to: 58-58
components/core/tests/test-GlobalMetadataDBConfig.cpp (5)
9-37: LGTM! Well-designed helper functions for testing.The helper functions provide clean abstractions for:
- Environment variable manipulation (
set_env_var,unset_env_var)- Command-line argument parsing using Boost.Program_options
- Proper const-correctness and template usage for compile-time array sizes
39-66: LGTM! Comprehensive test for command-line option parsing.The test properly validates that all database configuration parameters are correctly parsed from command-line arguments and stored in the configuration object.
68-80: LGTM! Proper test for environment variable credential reading.The test correctly verifies that database credentials are read from the expected environment variables and properly cleans up after itself.
82-126: LGTM! Thorough validation testing with proper error handling.The test comprehensively covers:
- Successful validation with complete MySQL configuration
- Proper error handling for missing credentials in various combinations
- Clean environment variable management in each test section
128-136: LGTM! Simple and effective SQLite configuration test.The test correctly verifies that SQLite configuration doesn't require credentials and validates successfully without them.
components/core/src/clp_s/indexer/CommandLineArguments.cpp (2)
32-32: LGTM! Proper delegation to centralized command-line option management.Using
GlobalMetadataDBConfig::add_command_line_options()ensures consistency across all components and centralizes the database configuration option definitions.
95-116: LGTM! Well-implemented configuration initialization and validation.The new configuration handling:
- Properly initializes from parsed command-line options
- Reads credentials from environment variables
- Validates the configuration with appropriate error handling
- Maintains the MySQL-only requirement for this component
- Provides clear error messages and logging
The error handling is consistent with the existing codebase patterns.
components/core/src/clp/GlobalMetadataDBConfig.hpp (4)
7-7: LGTM! Necessary include for Boost.Program_options integration.The inclusion of
boost/program_options.hppis required to support the new command-line argument parsing functionality.
21-28: LGTM! Well-designed static method for centralized option management.The static
add_command_line_optionsmethod provides a clean way to centralize database-related command-line option definitions across all components.
34-51: LGTM! Clean API design for the new configuration approach.The three-method approach provides good separation of concerns:
init_from_parsed_options: Handles command-line argument parsingread_credentials_from_env: Separates credential handling from other configvalidate: Ensures configuration completeness and correctnessThis design allows for flexible configuration workflows while maintaining clear responsibilities.
83-83: LGTM! Reasonable default host change.Changing the default from
"localhost"to"127.0.0.1"is a sensible choice that avoids DNS resolution and provides more predictable behaviour across different systems.docs/src/user-guide/core-unstructured/clp.md (2)
123-133: LGTM!The database configuration parameters are well documented with clear default values and environment variable requirements.
119-119: Consider adding a comma after "package".For better readability, consider adding a comma after "package".
-* Install the MariaDB Python package `pip3 install mariadb` +* Install the MariaDB Python package, `pip3 install mariadb`Likely an incorrect or invalid review comment.
components/core/src/clp/clg/CommandLineArguments.cpp (2)
44-51: LGTM!Clean integration of GlobalMetadataDBConfig command line options, properly delegating the responsibility to the config class.
191-199: LGTM!Proper error handling for metadata database configuration with clear initialization steps and appropriate error logging.
components/core/src/clp/clp/CommandLineArguments.cpp (1)
44-51: LGTM!The implementation is consistent with the clg command line parser, maintaining uniformity across the codebase.
Also applies to: 114-122
components/core/src/clp/GlobalMetadataDBConfig.cpp (4)
13-32: LGTM!Well-structured command line options with appropriate defaults and clear documentation for MySQL-specific parameters.
34-53: LGTM!Proper initialization logic that correctly handles different database types and only sets MySQL-specific parameters when appropriate.
55-65: LGTM!Safe implementation that properly checks for null pointers before reading environment variables.
67-94: Comprehensive validation logic.The validation method thoroughly checks all MySQL parameters with clear error messages. Good defensive programming.
components/job-orchestration/job_orchestration/executor/compress/compression_task.py (3)
185-199: Well-designed helper functions.The separation of database connection arguments and environment variables into dedicated helper functions improves code organization and reusability.
Also applies to: 201-212
252-256: LGTM!Clean integration of the database connection helpers, properly extending the command and environment variables.
456-473: LGTM!The indexer command properly uses the database connection helpers and sets environment variables consistently with the compression command.
junhaoliao
commented
Aug 1, 2025
| MetadataDBType m_metadata_db_type{MetadataDBType::SQLite}; | ||
|
|
||
| std::string m_metadata_db_host{"localhost"}; | ||
| std::string m_metadata_db_host{"127.0.0.1"}; |
Member
Author
There was a problem hiding this comment.
not sure if we want to change the default as 127.0.0.1 because accessing with host localhost would by default use sock connection, which isn't exposed by the package's start-clp script at least
Member
Author
There was a problem hiding this comment.
i can revert all related references if this change is confusing
Contributor
There was a problem hiding this comment.
I think 127.0.0.1 is reasonable, no need to revert.
junhaoliao
commented
Aug 1, 2025
junhaoliao
commented
Aug 1, 2025
junhaoliao
commented
Aug 1, 2025
junhaoliao
left a comment
Member
Author
There was a problem hiding this comment.
++
I kept making changes to get things work but later realized this PR has become a bit long especially after the addition of test cases and doc changes. Let me know if i should split the PR into smaller ones.
if not, owners kindly please help review the code
cc @gibber9809 for clp-s related code
cc @haiqi96 for the cli parser and the package scripts
cc @kirkrodrigues for the docs
Member
Author
|
converting to draft until CI finishes |
junhaoliao
commented
Aug 1, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
docs/src/user-guide/core-unstructured/clp.md (3)
109-109: Remove trailing space.Line 109 has a trailing space that should be removed for consistency.
-By default, `clp` uses an embedded SQLite database. Running multiple clp instances with SQLite +By default, `clp` uses an embedded SQLite database. Running multiple clp instances with SQLite
122-122: Remove trailing space.Line 122 has a trailing space that should be removed.
-* Run `tools/scripts/db/init-db.py`. This will initialize the database CLP requires. Use +* Run `tools/scripts/db/init-db.py`. This will initialize the database CLP requires. Use
131-131: Remove trailing space.Line 131 has a trailing space that should be removed.
-* Run `clp` or `clg` with the same command options and environment variables, with the addition of +* Run `clp` or `clg` with the same command options and environment variables, with the addition ofcomponents/core/src/clp/GlobalMetadataDBConfig.cpp (1)
16-31: Create a DB config specific subsection for better organization.As suggested in a previous review, consider creating a database configuration specific subsection instead of adding these options to the general options section. This would improve the organization of command-line help output.
auto GlobalMetadataDBConfig::add_command_line_options( boost::program_options::options_description& options_description ) -> void { - options_description.add_options() + boost::program_options::options_description db_options("Database options"); + db_options.add_options() ("db-type", boost::program_options::value<string>()->default_value("sqlite"), "Database type [sqlite | mysql]") // ... rest of options ... ("db-table-prefix", boost::program_options::value<string>()->default_value("clp_"), "[db-type=mysql] Database table prefix"); + options_description.add(db_options);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/core/src/clp/GlobalMetadataDBConfig.cpp(1 hunks)components/job-orchestration/job_orchestration/executor/compress/compression_task.py(5 hunks)docs/src/user-guide/core-unstructured/clp.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/src/clp/GlobalMetadataDBConfig.cpp
🧠 Learnings (23)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: haiqi96
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-07-29T14:04:13.769Z
Learning: User haiqi96 requested creating a GitHub issue to document a bug fix from PR #1136, which addressed MySQL compatibility issues with invalid SQL CAST operations in the WebUI component.
📚 Learning: in the clp project, when reviewing cmakelists.txt changes that introduce new compression library dep...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
docs/src/user-guide/core-unstructured/clp.mdcomponents/job-orchestration/job_orchestration/executor/compress/compression_task.py
📚 Learning: the clp project team prefers to use video content to demonstrate detailed procedural steps (like tar...
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in clp-s documentation, technical abbreviations like "mpt" (merged parse tree) should be defined at ...
Learnt from: quinntaylormitchell
PR: y-scope/clp#961
File: docs/src/dev-guide/design-clp-structured/single-file-archive-format.md:216-219
Timestamp: 2025-06-18T14:35:20.485Z
Learning: In clp-s documentation, technical abbreviations like "MPT" (Merged Parse Tree) should be defined at first use to improve reader clarity and comprehension.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: the clp project team has decided to refrain from using include directives in their documentation at ...
Learnt from: quinntaylormitchell
PR: y-scope/clp#1069
File: docs/src/user-guide/quick-start/clp-json.md:135-152
Timestamp: 2025-07-05T03:38:16.779Z
Learning: The CLP project team has decided to refrain from using include directives in their documentation at present, preferring to maintain duplicated content rather than using shared includes or partials for de-duplication.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in the clp-package compression flow, path validation and error handling is performed at the schedule...
Learnt from: haiqi96
PR: y-scope/clp#651
File: components/clp-package-utils/clp_package_utils/scripts/compress.py:0-0
Timestamp: 2025-01-16T16:58:43.190Z
Learning: In the clp-package compression flow, path validation and error handling is performed at the scheduler level rather than in the compress.py script to maintain simplicity and avoid code duplication.
Applied to files:
docs/src/user-guide/core-unstructured/clp.mdcomponents/job-orchestration/job_orchestration/executor/compress/compression_task.py
📚 Learning: in clp installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistenc...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: the y-scope/clp codebase has a 100-character line length limit that takes precedence over other styl...
Learnt from: junhaoliao
PR: y-scope/clp#1078
File: components/webui/server/src/plugins/MongoSocketIoServer/index.ts:105-108
Timestamp: 2025-07-08T17:20:02.560Z
Learning: The y-scope/clp codebase has a 100-character line length limit that takes precedence over other style preferences like template literals when they conflict.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: for installation scripts in the clp project, maintain consistency in command patterns across platfor...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in clp installation scripts, consistency across platform scripts is prioritized over defensive progr...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: when reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note th...
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: for installation scripts in the clp project, maintain consistency in command patterns across differe...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across different platforms (e.g., using separate update and install commands like `apk update && apk add`, `apt update && apt install`, `yum update && yum install`) rather than platform-specific optimizations, to ensure uniform script structure and readability.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in the clp project, the team prefers yoda-style conditions (e.g., `0 == len(dataset_name)`) over sta...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1100
File: integration-tests/tests/utils/config.py:42-45
Timestamp: 2025-07-28T23:14:57.751Z
Learning: In the CLP project, the team prefers Yoda-style conditions (e.g., `0 == len(dataset_name)`) over standard Python idioms to help catch accidental assignment operators (writing `=` instead of `==`). Additionally, the team finds using `not` for string emptiness checks ambiguous and prefers explicit length comparisons.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: for the clp-core debian package creation script, strict version format validation is considered unne...
Learnt from: jackluo923
PR: y-scope/clp#718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in the clp project, antlr code generation at build time is being removed by another pr. when reviewi...
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Applied to files:
docs/src/user-guide/core-unstructured/clp.mdcomponents/core/src/clp/GlobalMetadataDBConfig.cpp
📚 Learning: in the compression scheduler, the team prefers initializing in-memory caches from the database at st...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#831
File: components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py:0-0
Timestamp: 2025-04-17T16:55:06.658Z
Learning: In the compression scheduler, the team prefers initializing in-memory caches from the database at startup rather than performing repeated database queries for efficiency reasons. This approach maintains both performance and reliability across process restarts.
Applied to files:
docs/src/user-guide/core-unstructured/clp.mdcomponents/job-orchestration/job_orchestration/executor/compress/compression_task.py
📚 Learning: in `components/core/src/clp_s/jsonparser.cpp`, validation and exception throwing are unnecessary in ...
Learnt from: gibber9809
PR: y-scope/clp#630
File: components/core/src/clp_s/JsonParser.cpp:702-703
Timestamp: 2024-12-10T16:03:13.322Z
Learning: In `components/core/src/clp_s/JsonParser.cpp`, validation and exception throwing are unnecessary in the `get_archive_node_id` method when processing nodes, and should not be added.
Applied to files:
components/core/src/clp/GlobalMetadataDBConfig.cpp
📚 Learning: in `components/core/src/clp/bufferedfilereader.cpp`, refactoring the nested error handling condition...
Learnt from: haiqi96
PR: y-scope/clp#523
File: components/core/src/clp/BufferedFileReader.cpp:96-106
Timestamp: 2024-10-24T14:45:26.265Z
Learning: In `components/core/src/clp/BufferedFileReader.cpp`, refactoring the nested error handling conditions may not apply due to the specific logic in the original code.
Applied to files:
components/core/src/clp/GlobalMetadataDBConfig.cpp
📚 Learning: in `fs_compression_task.py`, when refactoring the archive processing loop in python, the `src_archiv...
Learnt from: haiqi96
PR: y-scope/clp#634
File: components/job-orchestration/job_orchestration/executor/compress/fs_compression_task.py:0-0
Timestamp: 2024-12-16T21:49:06.483Z
Learning: In `fs_compression_task.py`, when refactoring the archive processing loop in Python, the `src_archive_file.unlink()` operation should remain outside of the `process_archive` function.
Applied to files:
components/job-orchestration/job_orchestration/executor/compress/compression_task.py
📚 Learning: in `clp_package_utils/scripts/native/del_archives.py`, when deleting archives, the `archive` variabl...
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:104-110
Timestamp: 2024-11-15T16:21:52.122Z
Learning: In `clp_package_utils/scripts/native/del_archives.py`, when deleting archives, the `archive` variable retrieved from the database is controlled and is always a single string without path components. Therefore, it's acceptable to skip additional validation checks for directory traversal in this context.
Applied to files:
components/job-orchestration/job_orchestration/executor/compress/compression_task.py
📚 Learning: the column metadata table (created by `_create_column_metadata_table`) is only needed for dataset-sp...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#868
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py:141-144
Timestamp: 2025-05-05T16:32:55.163Z
Learning: The column metadata table (created by `_create_column_metadata_table`) is only needed for dataset-specific workflows in `CLP_S` and is obsolete for non-dataset workflows.
Applied to files:
components/job-orchestration/job_orchestration/executor/compress/compression_task.py
📚 Learning: in the clp codebase, `table_prefix` is a trusted configuration value and can be safely embedded into...
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/native/del_archives.py:90-96
Timestamp: 2024-11-15T16:22:33.635Z
Learning: In the CLP codebase, `table_prefix` is a trusted configuration value and can be safely embedded into SQL queries.
Applied to files:
components/job-orchestration/job_orchestration/executor/compress/compression_task.py
📚 Learning: in the clp codebase, the validate_and_cache_dataset function in components/clp-py-utils/clp_py_utils...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1036
File: components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py:204-211
Timestamp: 2025-07-03T12:58:18.407Z
Learning: In the CLP codebase, the validate_and_cache_dataset function in components/clp-py-utils/clp_py_utils/clp_metadata_db_utils.py uses in-place updates of the existing_datasets set parameter rather than returning a new set, as preferred by the development team.
Applied to files:
components/job-orchestration/job_orchestration/executor/compress/compression_task.py
🧬 Code Graph Analysis (1)
components/core/src/clp/GlobalMetadataDBConfig.cpp (1)
components/core/src/clp/GlobalMetadataDBConfig.hpp (2)
options_description(26-28)parsed_options(38-39)
🪛 LanguageTool
docs/src/user-guide/core-unstructured/clp.md
[style] ~117-~117: Consider using “who” when you are referring to a person instead of an object.
Context: ...tions for your platform * Create a user that has privileges to create databases, cre...
(THAT_WHO)
[uncategorized] ~119-~119: Possible missing comma found.
Context: ... records. * Install the MariaDB Python package pip3 install mariadb * This is nece...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (9)
docs/src/user-guide/core-unstructured/clp.md (2)
108-111: Clear warning about SQLite limitations.The warning effectively communicates the limitation of SQLite for parallel compression scenarios.
119-133: Well-documented database configuration options.The updated instructions clearly explain how to configure the database using command-line options and environment variables, including helpful default values for each option.
components/core/src/clp/GlobalMetadataDBConfig.cpp (3)
34-53: Proper parsing of database configuration options.The function correctly parses the database type and conditionally extracts MySQL-specific parameters. Good use of Yoda-style conditions as per project conventions.
55-65: Safe handling of environment variables.The function properly checks for null pointers when reading environment variables and only assigns values when they exist.
67-96: Comprehensive validation with clear error messages.The validation function properly checks all required MySQL parameters with appropriate error messages that reference the correct CLI options and environment variables.
components/job-orchestration/job_orchestration/executor/compress/compression_task.py (4)
185-201: Well-structured helper function for database arguments.The function cleanly extracts database connection parameters and formats them as command-line arguments, properly converting the port to a string.
204-214: Simple and effective credential mapping.The function correctly maps database credentials to the expected environment variable names.
255-260: Clean integration of database configuration.The changes properly integrate the new database configuration approach by adding CLI arguments and setting environment variables, effectively replacing the previous YAML-based configuration.
434-434: Proper subprocess configuration with new database parameters.The changes correctly configure the indexer subprocess with database connection arguments and environment variables, maintaining consistency with the new configuration approach.
Also applies to: 460-469
…p.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/src/user-guide/core-unstructured/clp.md(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 Learning: in the clp project, when reviewing cmakelists.txt changes that introduce new compression library dep...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: the clp project team prefers to use video content to demonstrate detailed procedural steps (like tar...
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in clp-s documentation, technical abbreviations like "mpt" (merged parse tree) should be defined at ...
Learnt from: quinntaylormitchell
PR: y-scope/clp#961
File: docs/src/dev-guide/design-clp-structured/single-file-archive-format.md:216-219
Timestamp: 2025-06-18T14:35:20.485Z
Learning: In clp-s documentation, technical abbreviations like "MPT" (Merged Parse Tree) should be defined at first use to improve reader clarity and comprehension.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: the clp project team has decided to refrain from using include directives in their documentation at ...
Learnt from: quinntaylormitchell
PR: y-scope/clp#1069
File: docs/src/user-guide/quick-start/clp-json.md:135-152
Timestamp: 2025-07-05T03:38:16.779Z
Learning: The CLP project team has decided to refrain from using include directives in their documentation at present, preferring to maintain duplicated content rather than using shared includes or partials for de-duplication.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in clp installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistenc...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in the clp-package compression flow, path validation and error handling is performed at the schedule...
Learnt from: haiqi96
PR: y-scope/clp#651
File: components/clp-package-utils/clp_package_utils/scripts/compress.py:0-0
Timestamp: 2025-01-16T16:58:43.190Z
Learning: In the clp-package compression flow, path validation and error handling is performed at the scheduler level rather than in the compress.py script to maintain simplicity and avoid code duplication.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: for installation scripts in the clp project, maintain consistency in command patterns across platfor...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in clp installation scripts, consistency across platform scripts is prioritized over defensive progr...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: the y-scope/clp codebase has a 100-character line length limit that takes precedence over other styl...
Learnt from: junhaoliao
PR: y-scope/clp#1078
File: components/webui/server/src/plugins/MongoSocketIoServer/index.ts:105-108
Timestamp: 2025-07-08T17:20:02.560Z
Learning: The y-scope/clp codebase has a 100-character line length limit that takes precedence over other style preferences like template literals when they conflict.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in the clp project, antlr code generation at build time is being removed by another pr. when reviewi...
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: when reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note th...
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in the compression scheduler, the team prefers initializing in-memory caches from the database at st...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#831
File: components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py:0-0
Timestamp: 2025-04-17T16:55:06.658Z
Learning: In the compression scheduler, the team prefers initializing in-memory caches from the database at startup rather than performing repeated database queries for efficiency reasons. This approach maintains both performance and reliability across process restarts.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
🪛 LanguageTool
docs/src/user-guide/core-unstructured/clp.md
[uncategorized] ~119-~119: Possible missing comma found.
Context: ... records. * Install the MariaDB Python package pip3 install mariadb * This is nece...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (2)
docs/src/user-guide/core-unstructured/clp.md (2)
125-134: Double-check option names stay aligned with the C++ & Python parsersThe documentation lists
--db-nameand--db-table-prefix. A quick scan of the C++ changes showed those exact flags, but please verify no last-minute renames (e.g.,--db-database) slipped in; mismatches here lead to painful runtime errors.
119-120: Tighten wording & punctuation in the Python-package bulletA small wording tweak plus a colon after “package” reads more naturally and satisfies markdown-lint for the “missing comma” hint.
-* Install the MariaDB Python package `pip3 install mariadb` - * This is necessary to run the database initialization script. If you prefer, you can run the SQL - statements in `tools/scripts/db/init-db.py` directly. +* Install the MariaDB Python package: `pip3 install mariadb` + * This package is required to run the database-initialisation script. If you prefer, you can + execute the SQL statements in `tools/scripts/db/init-db.py` directly.Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/src/user-guide/core-unstructured/clp.md(1 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: haiqi96
PR: y-scope/clp#0
File: :0-0
Timestamp: 2025-07-29T14:04:13.769Z
Learning: User haiqi96 requested creating a GitHub issue to document a bug fix from PR #1136, which addressed MySQL compatibility issues with invalid SQL CAST operations in the WebUI component.
📚 Learning: in the clp project, when reviewing cmakelists.txt changes that introduce new compression library dep...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: the clp project team prefers to use video content to demonstrate detailed procedural steps (like tar...
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in clp-s documentation, technical abbreviations like "mpt" (merged parse tree) should be defined at ...
Learnt from: quinntaylormitchell
PR: y-scope/clp#961
File: docs/src/dev-guide/design-clp-structured/single-file-archive-format.md:216-219
Timestamp: 2025-06-18T14:35:20.485Z
Learning: In clp-s documentation, technical abbreviations like "MPT" (Merged Parse Tree) should be defined at first use to improve reader clarity and comprehension.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: the clp project team has decided to refrain from using include directives in their documentation at ...
Learnt from: quinntaylormitchell
PR: y-scope/clp#1069
File: docs/src/user-guide/quick-start/clp-json.md:135-152
Timestamp: 2025-07-05T03:38:16.779Z
Learning: The CLP project team has decided to refrain from using include directives in their documentation at present, preferring to maintain duplicated content rather than using shared includes or partials for de-duplication.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in clp installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistenc...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: for installation scripts in the clp project, maintain consistency in command patterns across platfor...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in clp installation scripts, consistency across platform scripts is prioritized over defensive progr...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: the y-scope/clp codebase has a 100-character line length limit that takes precedence over other styl...
Learnt from: junhaoliao
PR: y-scope/clp#1078
File: components/webui/server/src/plugins/MongoSocketIoServer/index.ts:105-108
Timestamp: 2025-07-08T17:20:02.560Z
Learning: The y-scope/clp codebase has a 100-character line length limit that takes precedence over other style preferences like template literals when they conflict.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in the clp project, antlr code generation at build time is being removed by another pr. when reviewi...
Learnt from: gibber9809
PR: y-scope/clp#955
File: components/core/src/clp_s/search/sql/CMakeLists.txt:8-26
Timestamp: 2025-06-02T18:22:24.060Z
Learning: In the clp project, ANTLR code generation at build time is being removed by another PR. When reviewing CMake files, be aware that some temporary suboptimal configurations may exist to reduce merge conflicts between concurrent PRs, especially around ANTLR_TARGET calls.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: when reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note th...
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in the clp project, the team prefers yoda-style conditions (e.g., `0 == len(dataset_name)`) over sta...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1100
File: integration-tests/tests/utils/config.py:42-45
Timestamp: 2025-07-28T23:14:57.751Z
Learning: In the CLP project, the team prefers Yoda-style conditions (e.g., `0 == len(dataset_name)`) over standard Python idioms to help catch accidental assignment operators (writing `=` instead of `==`). Additionally, the team finds using `not` for string emptiness checks ambiguous and prefers explicit length comparisons.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: for installation scripts in the clp project, prefer explicit error handling over automatic dependenc...
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.408Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in the clp project, sql queries should use parameterized queries with placeholders (%s) and pass val...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#831
File: components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py:0-0
Timestamp: 2025-04-17T16:55:23.796Z
Learning: In the CLP project, SQL queries should use parameterized queries with placeholders (%s) and pass values as a tuple to `db_cursor.execute()` to prevent SQL injection, rather than directly interpolating values into the query string.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
📚 Learning: in the compression scheduler, the team prefers initializing in-memory caches from the database at st...
Learnt from: Bill-hbrhbr
PR: y-scope/clp#831
File: components/job-orchestration/job_orchestration/scheduler/compress/compression_scheduler.py:0-0
Timestamp: 2025-04-17T16:55:06.658Z
Learning: In the compression scheduler, the team prefers initializing in-memory caches from the database at startup rather than performing repeated database queries for efficiency reasons. This approach maintains both performance and reliability across process restarts.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
🪛 LanguageTool
docs/src/user-guide/core-unstructured/clp.md
[uncategorized] ~118-~118: Possible missing comma found.
Context: ... records. * Install the MariaDB Python package pip3 install mariadb * This is nece...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (1)
docs/src/user-guide/core-unstructured/clp.md (1)
108-111: Warning block now reads clearly – good improvementThe wording succinctly explains why concurrent writes fail with SQLite and correctly back-ticks
clp.
Comment on lines
+121
to
+130
| * Run `tools/scripts/db/init-db.py`. This will initialize the database CLP requires. Use | ||
| command-line options to configure MySQL connection parameters and set environment variables | ||
| for database credentials: | ||
| * `--db-host <host>` to specify the database host (default: `"localhost"`) | ||
| * `--db-port <port>` to specify the database port (default: `3306`) | ||
| * `--db-name <name>` to specify the database name (default: `"clp-db"`) | ||
| * `--db-table-prefix <prefix>` to specify the table prefix (default: `"clp_"`) | ||
| * Set `CLP_DB_USER` environment variable for the database username | ||
| * Set `CLP_DB_PASS` environment variable for the database password | ||
| * Run `clp` or `clg` with the same command options and environment variables, with the addition of |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding a concrete export/usage snippet for the env vars
Readers unfamiliar with environment variables may appreciate a quick, copy-pasta example showing how to set CLP_DB_USER/CLP_DB_PASS and pass the matching CLI flags. This can go right after the last bullet to keep the flow tight.
* Set `CLP_DB_PASS` environment variable for the database password
+
+```shell
+# Example
+export CLP_DB_USER=clpuser
+export CLP_DB_PASS='s3cr3t!'
+./tools/scripts/db/init-db.py --db-host localhost --db-name clp-db
+```🤖 Prompt for AI Agents
In docs/src/user-guide/core-unstructured/clp.md around lines 121 to 130, add a
concrete example snippet demonstrating how to set the CLP_DB_USER and
CLP_DB_PASS environment variables and run the init-db.py script with matching
CLI flags. Insert a shell code block after the last bullet point showing export
commands for the environment variables and a sample command line invocation of
init-db.py to help users unfamiliar with environment variables quickly
understand usage.
junhaoliao
commented
Aug 1, 2025
gibber9809
requested changes
Aug 1, 2025
gibber9809
left a comment
Contributor
There was a problem hiding this comment.
Partial review for
- c++ and cmake code
- init-db.py
I haven't looked at compression_task.py or clp.md.
Generally looks good besides a few small comments. I think we might also be able to remove GlobalMetadataDBConfig.cpp from the exceptions in taskfiles/lint.yaml -- once we remove that unused using I pointed out I think it should pass clang-tidy.
| MetadataDBType m_metadata_db_type{MetadataDBType::SQLite}; | ||
|
|
||
| std::string m_metadata_db_host{"localhost"}; | ||
| std::string m_metadata_db_host{"127.0.0.1"}; |
Contributor
There was a problem hiding this comment.
I think 127.0.0.1 is reasonable, no need to revert.
Comment on lines
+17
to
+31
| ("db-type", | ||
| boost::program_options::value<string>()->default_value("sqlite"), | ||
| "Database type [sqlite | mysql]") | ||
| ("db-host", | ||
| boost::program_options::value<string>()->default_value("127.0.0.1"), | ||
| "[db-type=mysql] Database host") | ||
| ("db-port", | ||
| boost::program_options::value<int>()->default_value(cDefaultMetadataDbPort), | ||
| "[db-type=mysql] Database port") | ||
| ("db-name", | ||
| boost::program_options::value<string>()->default_value("clp-db"), | ||
| "[db-type=mysql] Database name") | ||
| ("db-table-prefix", | ||
| boost::program_options::value<string>()->default_value("clp_"), | ||
| "[db-type=mysql] Database table prefix"); |
Contributor
There was a problem hiding this comment.
I think it might be a bit cleaner if we make this non-static and bind these directly to member variables e.g.
"db-host", boost::program_options::value<std::string>(&m_metadata_db_host)->default_value(m_metadata_db_host),...
and then change init_from_parsed_options(...) to validate_parsed_options().
That way we can rely on the default values in the header for the default values in the command line options instead of having to repeat them here + parse the variables by name from the variables_map later.
Contributor
There was a problem hiding this comment.
Or rather I guess we could just merge the bit of validation that init_from_parsed_options does into validate.
| std::string m_metadata_db_host{"localhost"}; | ||
| std::string m_metadata_db_host{"127.0.0.1"}; | ||
| int m_metadata_db_port{cDefaultMetadataDbPort}; | ||
| std::string m_metadata_db_name; |
Contributor
There was a problem hiding this comment.
If you take the suggestion from my other comment we could just set all of the default values here using {}.
Member
Author
There was a problem hiding this comment.
I extracted the default values as constants, and in add_command_lines_options i specified the arguments' default values as those extracted constants, just in case someone calls add_command_lines_options() multiple times. (i feel it's unlikely someone would call that method multiple times but since we didn't really say that's prohibited in the docs, we should try to handle such call sequences)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (9)
docs/src/user-guide/core-unstructured/clp.md (3)
119-120: Use “who” when referring to a user (person).-* Create a user that has privileges to create databases, create tables, insert records, and delete +* Create a user who has privileges to create databases, create tables, insert records, and delete
131-132: Simplify phrasing and add a brief security tip for CLP_DB_PASS.Removes redundancy and helps users avoid leaking secrets in shell history.
- * Set the `CLP_DB_USER` environment variable for the database user's username - * Set the `CLP_DB_PASS` environment variable for the database user's password + * Set the `CLP_DB_USER` environment variable for the database username + * Set the `CLP_DB_PASS` environment variable for the database password + * Tip: Avoid exposing the password in shell history; consider reading it interactively (e.g., `read -s`) + or using a secrets manager.
116-116: Clarify both tools must target the same DB instance.Avoids misconfigurations when users point clp and clg to different DBs.
-You can configure `clp` and `clg` to use a MySQL-type database as follows: +You can configure `clp` and `clg` to use a MySQL-type database as follows. Both tools must point to +the same database instance initialised in the next step:components/core/src/clp_s/indexer/IndexManager.hpp (1)
42-46: Clarify ctor docs: explicitly state that std::nullopt will throwThe default value is std::nullopt, and the implementation throws on missing/invalid config. Please make the behaviour explicit in the comment to prevent misuse.
- /** - * @param optional_db_config - * @param should_create_table - * @throw OperationFailed if `optional_db_config` is invalid. - */ + /** + * @param optional_db_config Metadata DB configuration. Passing std::nullopt will cause this + * constructor to throw OperationFailed. + * @param should_create_table Whether to create the index table if it does not exist. + * @throws OperationFailed if `optional_db_config` is std::nullopt or otherwise invalid. + */components/core/tests/test-GlobalMetadataDBConfig.cpp (1)
140-144: Test incorrectly expects validation to fail when only username is providedAccording to the implementation, when only username is set (without password), it should be valid since password can be empty. The current test expects it to throw, which appears incorrect.
SECTION("Username set but password missing") { set_env_var("CLP_DB_USER", std::string{cMysqlDbUser}.c_str()); config.read_credentials_from_env_if_needed(); - REQUIRE_THROWS_AS(config.validate(), std::invalid_argument); + // Password can be empty, so this should not throw + REQUIRE_NOTHROW(config.validate()); + REQUIRE((config.get_metadata_db_username() == cMysqlDbUser)); + REQUIRE(false == config.get_metadata_db_password().has_value()); }components/core/src/clp/global_metadata_db_utils.hpp (1)
17-17: Clarify the return contract in documentationThe factory function can return nullptr on failure, but this isn't mentioned in the documentation. This could lead to confusion for users of the API.
- * @return The created GlobalMetadataDB instance. + * @return The created GlobalMetadataDB instance, or nullptr on failure.components/core/src/clp/clg/clg.cpp (1)
12-12: Remove unused DB backend includes after factory adoptionThese headers are no longer needed in this TU and add unnecessary compile-time coupling.
#include "../global_metadata_db_utils.hpp" -#include "../GlobalMySQLMetadataDB.hpp" -#include "../GlobalSQLiteMetadataDB.hpp"components/core/src/clp/clp/compression.cpp (1)
10-10: Remove unused DB backend includes after factory adoptionAfter switching to create_global_metadata_db, these direct backend headers can be dropped.
#include "../global_metadata_db_utils.hpp" -#include "../GlobalMySQLMetadataDB.hpp" -#include "../GlobalSQLiteMetadataDB.hpp"components/core/src/clp/global_metadata_db_utils.cpp (1)
55-62: Prefer handling all enum cases explicitly; drop default to catch omissions at compile timeUsing a default hides newly added enum values from compiler diagnostics. Remove default, and add a single post-switch fallback that reuses the common error prefix. This also eliminates the now-redundant trailing return.
Apply this diff:
- default: - SPDLOG_ERROR( - "Unhandled metadata DB type: {}", - static_cast<int>(metadata_db_config.get_metadata_db_type()) - ); - return {}; - } - return {}; + } + // This should be unreachable if all enum cases are handled above. + SPDLOG_ERROR( + "{}: Unhandled metadata DB type: {}", + cErrorMsgPrefix, + static_cast<int>(metadata_db_config.get_metadata_db_type()) + ); + return {};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
components/core/src/clp/clg/clg.cpp(2 hunks)components/core/src/clp/clp/compression.cpp(3 hunks)components/core/src/clp/global_metadata_db_utils.cpp(1 hunks)components/core/src/clp/global_metadata_db_utils.hpp(1 hunks)components/core/src/clp_s/indexer/IndexManager.hpp(1 hunks)components/core/tests/test-GlobalMetadataDBConfig.cpp(1 hunks)docs/src/user-guide/core-unstructured/clp.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/src/clp/clg/clg.cppcomponents/core/src/clp/global_metadata_db_utils.hppcomponents/core/tests/test-GlobalMetadataDBConfig.cppcomponents/core/src/clp/global_metadata_db_utils.cppcomponents/core/src/clp_s/indexer/IndexManager.hppcomponents/core/src/clp/clp/compression.cpp
🧠 Learnings (4)
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
docs/src/user-guide/core-unstructured/clp.mdcomponents/core/src/clp/clp/compression.cpp
📚 Learning: 2024-11-01T03:26:26.386Z
Learnt from: LinZhihao-723
PR: y-scope/clp#570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
Applied to files:
components/core/tests/test-GlobalMetadataDBConfig.cpp
📚 Learning: 2024-12-05T16:32:21.507Z
Learnt from: haiqi96
PR: y-scope/clp#619
File: components/core/src/clp/clp/decompression.cpp:313-313
Timestamp: 2024-12-05T16:32:21.507Z
Learning: In the C++ `FileDecompressor` implementations at `components/core/src/clp/clp/FileDecompressor.cpp` and `components/core/src/glt/glt/FileDecompressor.cpp`, the `temp_output_path` variable and associated logic are used to handle multiple compressed files with the same name, and should be kept. This logic is separate from the temporary output directory code removed in PR #619 and is necessary for proper file handling.
Applied to files:
components/core/src/clp/clp/compression.cpp
📚 Learning: 2024-11-19T17:30:04.970Z
Learnt from: AVMatthews
PR: y-scope/clp#595
File: components/core/tests/test-end_to_end.cpp:59-65
Timestamp: 2024-11-19T17:30:04.970Z
Learning: In 'components/core/tests/test-end_to_end.cpp', during the 'clp-s_compression_and_extraction_no_floats' test, files and directories are intentionally removed at the beginning of the test to ensure that any existing content doesn't influence the test results.
Applied to files:
components/core/src/clp/clp/compression.cpp
🧬 Code Graph Analysis (2)
components/core/src/clp/clg/clg.cpp (2)
components/core/src/clp/global_metadata_db_utils.cpp (2)
create_global_metadata_db(17-63)create_global_metadata_db(17-20)components/core/src/clp/global_metadata_db_utils.hpp (1)
create_global_metadata_db(19-22)
components/core/src/clp/clp/compression.cpp (4)
components/core/src/clp/GlobalSQLiteMetadataDB.hpp (2)
path(92-96)path(92-94)components/core/src/clp/GlobalMetadataDB.hpp (1)
path(91-91)components/core/src/clp/global_metadata_db_utils.cpp (2)
create_global_metadata_db(17-63)create_global_metadata_db(17-20)components/core/src/clp/global_metadata_db_utils.hpp (1)
create_global_metadata_db(19-22)
🪛 LanguageTool
docs/src/user-guide/core-unstructured/clp.md
[style] ~119-~119: Consider using “who” when you are referring to a person instead of an object.
Context: ...tions for your platform * Create a user that has privileges to create databases, cre...
(THAT_WHO)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build (macos-15)
- GitHub Check: lint-check (macos-15)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
🔇 Additional comments (6)
docs/src/user-guide/core-unstructured/clp.md (2)
112-114: LGTM: concise warning matches the new configuration model.Clear, actionable warning; aligns with the removal of YAML config and MySQL-type guidance.
137-137: LGTM: parallelism guidance is now scoped to clp only.Accurately reflects the current single-instance limitation of clg and clp x.
components/core/src/clp/clg/clg.cpp (1)
537-541: Use raw pointer correctly with factoryThe factory returns a
std::unique_ptr<GlobalMetadataDB>, but the code treats it as a raw pointer. The null check is correct, but the variable declaration should reflect the actual return type.- auto global_metadata_db - = create_global_metadata_db(command_line_args.get_metadata_db_config(), archives_dir); - if (nullptr == global_metadata_db) { + auto global_metadata_db_ptr + = create_global_metadata_db(command_line_args.get_metadata_db_config(), archives_dir); + if (nullptr == global_metadata_db_ptr) { return -1; } - global_metadata_db->open(); + global_metadata_db_ptr->open();And update the usage on line 554:
- *global_metadata_db, + *global_metadata_db_ptr,Likely an incorrect or invalid review comment.
components/core/src/clp/clp/compression.cpp (1)
74-78: LGTM! Clean factory usage with proper error handlingThe factory pattern adoption is well-implemented with appropriate null pointer checking and early return on failure.
components/core/src/clp/global_metadata_db_utils.cpp (2)
23-28: Good: optional validated before value() accessGuarding the optional prevents std::bad_optional_access and makes failure explicit with a clear log message.
31-33: SQLite path resolution looks correctUsing archives_dir combined with streaming_archive::cMetadataDBFileName is appropriate here.
Comment on lines
+16
to
+20
| namespace clp { | ||
| auto create_global_metadata_db( | ||
| std::optional<GlobalMetadataDBConfig> const& optional_metadata_db_config, | ||
| std::filesystem::path const& archives_dir | ||
| ) -> std::unique_ptr<GlobalMetadataDB> { |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure unit tests cover failure paths of the factory
Given the new factory, please confirm tests cover:
- No config provided -> returns nullptr and logs error
- MySQL with missing or empty CLP_DB_USER/CLP_DB_PASS -> returns nullptr and logs actionable error
- Valid SQLite config -> returns GlobalSQLiteMetadataDB
- Valid MySQL config -> returns GlobalMySQLMetadataDB
- Unknown/unsupported DB type -> logs and returns nullptr
Run this quick scan to see if tests reference the factory and MySQL paths; I can help add missing cases.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for tests referencing create_global_metadata_db..."
rg -n --hidden --glob '!*node_modules/*' $'create_global_metadata_db\\(' -A 3 || true
echo
echo "Searching for tests referencing MySQL config paths or env vars..."
rg -n --hidden --glob '!*node_modules/*' $'MetadataDBType::MySQL|CLP_DB_USER|CLP_DB_PASS' -A 2 || trueLength of output: 16291
Add unit tests for create_global_metadata_db
The scans didn’t find any existing tests covering the new factory in global_metadata_db_utils.cpp. Please add tests for:
- No config provided → returns nullptr and logs an error
- MySQL config with missing or empty CLP_DB_USER/CLP_DB_PASS → returns nullptr and logs a clear error
- Valid SQLite config → returns a GlobalSQLiteMetadataDB instance
- Valid MySQL config → returns a GlobalMySQLMetadataDB instance
- Unknown/unsupported DB type → logs and returns nullptr
You can add these in a new test file (e.g. components/core/tests/test-global_metadata_db_utils.cpp).
Comment on lines
+29
to
+54
| switch (metadata_db_config.get_metadata_db_type()) { | ||
| case GlobalMetadataDBConfig::MetadataDBType::SQLite: { | ||
| auto global_metadata_db_path = archives_dir / streaming_archive::cMetadataDBFileName; | ||
| return std::make_unique<GlobalSQLiteMetadataDB>(global_metadata_db_path.string()); | ||
| } | ||
| case GlobalMetadataDBConfig::MetadataDBType::MySQL: { | ||
| auto const& metadata_db_username = metadata_db_config.get_metadata_db_username(); | ||
| auto const& metadata_db_password = metadata_db_config.get_metadata_db_password(); | ||
| if (false == metadata_db_username.has_value() | ||
| || false == metadata_db_password.has_value()) | ||
| { | ||
| SPDLOG_ERROR( | ||
| "{}: Global MySQL metadata DB credentials unexpectedly not set.", | ||
| cErrorMsgPrefix | ||
| ); | ||
| return {}; | ||
| } | ||
| return std::make_unique<GlobalMySQLMetadataDB>( | ||
| metadata_db_config.get_metadata_db_host(), | ||
| metadata_db_config.get_metadata_db_port(), | ||
| metadata_db_username.value(), | ||
| metadata_db_password.value(), | ||
| metadata_db_config.get_metadata_db_name(), | ||
| metadata_db_config.get_metadata_table_prefix() | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: catch constructor exceptions and log
If GlobalSQLiteMetadataDB or GlobalMySQLMetadataDB can throw during construction (e.g., invalid params, connection failure), consider wrapping the switch body in a try/catch and logging the exception to keep the factory’s contract of “return nullptr on failure” consistent.
Example (illustrative, adjust scope as needed):
try {
// switch with returns
} catch (std::exception const& e) {
SPDLOG_ERROR("{}: Exception while creating metadata DB: {}", cErrorMsgPrefix, e.what());
return {};
}
🤖 Prompt for AI Agents
In components/core/src/clp/global_metadata_db_utils.cpp around lines 29 to 54,
the factory creates GlobalSQLiteMetadataDB and GlobalMySQLMetadataDB instances
but does not guard against exceptions thrown by their constructors; wrap the
switch (or at least the constructor calls) in a try/catch that catches
std::exception (and optionally std::... for safety), log the exception with
SPDLOG_ERROR including cErrorMsgPrefix and e.what(), and return {} on any caught
exception so the function consistently returns nullptr on failure.
Comment on lines
+37
to
+45
| if (false == metadata_db_username.has_value() | ||
| || false == metadata_db_password.has_value()) | ||
| { | ||
| SPDLOG_ERROR( | ||
| "{}: Global MySQL metadata DB credentials unexpectedly not set.", | ||
| cErrorMsgPrefix | ||
| ); | ||
| return {}; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also reject empty MySQL credentials and guide users to env vars
You already check presence (has_value). It’s worth also rejecting empty strings, as CLP_DB_USER/CLP_DB_PASS might be defined but empty, leading to confusing connection failures downstream. The error message can be more actionable by naming the expected env vars.
Apply this diff:
- if (false == metadata_db_username.has_value()
- || false == metadata_db_password.has_value())
- {
- SPDLOG_ERROR(
- "{}: Global MySQL metadata DB credentials unexpectedly not set.",
- cErrorMsgPrefix
- );
- return {};
- }
+ if (false == metadata_db_username.has_value()
+ || false == metadata_db_password.has_value()
+ || 0 == metadata_db_username->size()
+ || 0 == metadata_db_password->size())
+ {
+ SPDLOG_ERROR(
+ "{}: Global MySQL metadata DB credentials not provided. "
+ "Expected env vars CLP_DB_USER and CLP_DB_PASS to be set and non-empty.",
+ cErrorMsgPrefix
+ );
+ return {};
+ }🤖 Prompt for AI Agents
In components/core/src/clp/global_metadata_db_utils.cpp around lines 37 to 45,
the code only checks has_value() for metadata_db_username and
metadata_db_password but does not reject empty strings; update the guard to also
treat empty values as invalid (e.g., metadata_db_username.has_value() &&
!metadata_db_username->empty()) and similarly for password, and change the
SPDLOG_ERROR message to explicitly list the expected environment variables
(CLP_DB_USER and CLP_DB_PASS) so the log guides users to set those vars; keep
the same return {} behavior when credentials are missing/empty.
Comment on lines
+57
to
+67
| void set_env_var(char const* name, char const* value) { | ||
| // Silence the check since this class won't be used in a multithreaded context. | ||
| // NOLINTNEXTLINE(concurrency-mt-unsafe) | ||
| setenv(name, value, 1); | ||
| } | ||
|
|
||
| void unset_env_var(char const* name) { | ||
| // Silence the check since this class won't be used in a multithreaded context. | ||
| // NOLINTNEXTLINE(concurrency-mt-unsafe) | ||
| unsetenv(name); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using RAII for environment variable cleanup
The manual environment variable management could miss cleanup if tests throw exceptions. Consider using a RAII wrapper for safer cleanup.
class EnvVarGuard {
public:
explicit EnvVarGuard(char const* name) : m_name(name) {}
~EnvVarGuard() { unset_env_var(m_name); }
private:
char const* m_name;
};Then use it in tests:
EnvVarGuard user_guard("CLP_DB_USER");
EnvVarGuard pass_guard("CLP_DB_PASS");
set_env_var("CLP_DB_USER", cMysqlDbUser.data());
// ... test code ...
// Automatic cleanup on scope exit🤖 Prompt for AI Agents
In components/core/tests/test-GlobalMetadataDBConfig.cpp around lines 57 to 67,
replace the manual set_env_var/unset_env_var usage in tests with a small RAII
guard class to ensure cleanup on scope exit; add an EnvVarGuard that stores the
env var name (and optionally the previous value if you want restore semantics)
and calls unset_env_var(m_name) in its destructor, then create EnvVarGuard
instances immediately after setting each environment variable (e.g., after
set_env_var("CLP_DB_USER", ...)) so cleanup is automatic even if the test throws
or returns early.
Comment on lines
+108
to
+110
| To enable parallel compression to the same archives directory, `clp`/`clg` need to be configured to | ||
| use a MySQL-type database (e.g., MariaDB) rather than the default---an embedded SQLite database | ||
| (which doesn't support concurrent writes). |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Limit “parallel compression” phrasing to clp (clg isn’t part of compression).
This sentence conflates clp and clg for a compression-only concern. Tighten it to clp for clarity and slightly improve phrasing.
-To enable parallel compression to the same archives directory, `clp`/`clg` need to be configured to
-use a MySQL-type database (e.g., MariaDB) rather than the default---an embedded SQLite database
-(which doesn't support concurrent writes).
+To enable parallel compression into the same archives directory, `clp` needs to be configured to use
+a MySQL-type database (e.g., MariaDB) rather than the default—an embedded SQLite database (which
+doesn't support concurrent writes).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| To enable parallel compression to the same archives directory, `clp`/`clg` need to be configured to | |
| use a MySQL-type database (e.g., MariaDB) rather than the default---an embedded SQLite database | |
| (which doesn't support concurrent writes). | |
| To enable parallel compression into the same archives directory, `clp` needs to be configured to use | |
| a MySQL-type database (e.g., MariaDB) rather than the default—an embedded SQLite database (which | |
| doesn't support concurrent writes). |
🤖 Prompt for AI Agents
In docs/src/user-guide/core-unstructured/clp.md around lines 108 to 110, the
sentence wrongly groups clp and clg under a compression-specific requirement;
change the phrasing to refer only to clp and clarify that clp must be configured
to use a MySQL-compatible database (e.g., MariaDB) instead of the default
embedded SQLite to enable parallel compression to the same archives directory,
removing any mention of clg and tightening the wording for clarity.
Comment on lines
+133
to
+135
| * Run `clp` or `clg` with the same command-line options and environment variables, with the addition | ||
| of the database type command-line option: | ||
| * `--db-type mysql` to specify MySQL as the database type |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add a copy-paste example block after the db-type bullet.
A short, concrete snippet reduces setup friction and demonstrates the new CLI/env flow.
* `--db-type mysql` to specify MySQL as the database type
+
+```shell
+# Example setup
+export CLP_DB_USER=clpuser
+read -s -p "DB password: " CLP_DB_PASS && echo
+export CLP_DB_PASS
+
+# Initialise the DB schema (adjust as needed)
+./tools/scripts/db/init-db.py --db-host 127.0.0.1 --db-port 3306 --db-name clp-db --db-table-prefix clp_
+
+# Run clp using a MySQL-type database
+./clp c --db-type mysql /path/to/archives /path/to/logs
+```🤖 Prompt for AI Agents
In docs/src/user-guide/core-unstructured/clp.md around lines 133 to 135, add a
copy-paste example block immediately after the db-type bullet showing a concrete
shell snippet; insert a fenced code block labeled "shell" containing the example
environment exports (CLP_DB_USER, read-s password into CLP_DB_PASS and export
it), the DB schema init command (init-db.py with host/port/name/table-prefix
placeholders), and a final example of running ./clp with the --db-type mysql
flag and paths; ensure the block is fenced with triple backticks and placed
directly after the bullet so it renders as a single example block in the docs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
components/core/tests/test-GlobalMetadataDBConfig.cpp (1)
56-66: Stabilize env var handling with RAII to avoid cross-section leakage.setenv/unsetenv are process-scoped. Catch2 sections run in separate paths within the same process, so leaked env state can cause flaky interactions. Wrap env mutations with an RAII guard to guarantee cleanup, even on exceptions.
+class EnvVarGuard { +public: + explicit EnvVarGuard(char const* name) + : m_name(name) + , m_had_prev(nullptr != std::getenv(name)) { + if (m_had_prev) { + m_prev = std::getenv(name); + } + } + void set(char const* value) const { + // NOLINTNEXTLINE(concurrency-mt-unsafe) + setenv(m_name, value, 1); + } + ~EnvVarGuard() { + if (m_had_prev) { + // NOLINTNEXTLINE(concurrency-mt-unsafe) + setenv(m_name, m_prev.c_str(), 1); + } else { + // NOLINTNEXTLINE(concurrency-mt-unsafe) + unsetenv(m_name); + } + } +private: + char const* m_name; + bool m_had_prev; + std::string m_prev; +};Example usage within a SECTION (outside this hunk):
EnvVarGuard user_guard("CLP_DB_USER"); EnvVarGuard pass_guard("CLP_DB_PASS"); user_guard.set(std::string{cMysqlDbUser}.c_str()); pass_guard.set(std::string{cMysqlDbPass}.c_str()); // ... test assertions ...You can then drop the manual “Tear down” unsets at the end of the test cases.
docs/src/user-guide/core-unstructured/clp.md (4)
133-137: Align parallel-run guidance with single-instance clg note below.This section should avoid implying clg can be parallelised (Line 139/140 state clg is single-instance).
-* Run `clp` or `clg` with the same command-line options and environment variables, with the addition +* Run `clp` with the same command-line options and environment variables, with the addition of the database type command-line option: * `--db-type mysql` to specify MySQL as the database type -To compress logs in parallel, run as many parallel instances of `clp` as desired. +To compress logs in parallel, run as many parallel instances of `clp` as desired.
131-132: Simplify phrasing: “database username/password.”“User’s username/password” is redundant.
- * Set the `CLP_DB_USER` environment variable for the database user's username - * Set the `CLP_DB_PASS` environment variable for the database user's password + * Set the `CLP_DB_USER` environment variable for the database username + * Set the `CLP_DB_PASS` environment variable for the database password
119-121: Use “who” when referring to a person.-* Create a user that has privileges to create databases, create tables, insert records, and delete +* Create a user who has privileges to create databases, create tables, insert records, and delete records.
108-110: Limit this statement to clp to avoid confusion.Search (clg) doesn’t affect compression concurrency. Suggest focusing the requirement on clp here.
-To enable parallel compression to the same archives directory, `clp` (and by extension, `clg`) needs +To enable parallel compression to the same archives directory, `clp` needs to be configured to use a MySQL-type database (e.g., MariaDB) rather than the default---an embedded SQLite database (which doesn't support concurrent writes).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/core/src/clp/global_metadata_db_utils.hpp(1 hunks)components/core/tests/test-GlobalMetadataDBConfig.cpp(1 hunks)docs/src/user-guide/core-unstructured/clp.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}
⚙️ CodeRabbit Configuration File
- Prefer
false == <expression>rather than!<expression>.
Files:
components/core/src/clp/global_metadata_db_utils.hppcomponents/core/tests/test-GlobalMetadataDBConfig.cpp
🧠 Learnings (2)
📚 Learning: 2024-11-01T03:26:26.386Z
Learnt from: LinZhihao-723
PR: y-scope/clp#570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
Applied to files:
components/core/tests/test-GlobalMetadataDBConfig.cpp
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
🪛 LanguageTool
docs/src/user-guide/core-unstructured/clp.md
[style] ~119-~119: Consider using “who” when you are referring to a person instead of an object.
Context: ...tions for your platform * Create a user that has privileges to create databases, cre...
(THAT_WHO)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
🔇 Additional comments (5)
components/core/src/clp/global_metadata_db_utils.hpp (1)
4-10: Includes and guard look correct; factory signature is clear.STL headers are explicitly included, local headers follow, and the header guard is conventional. The return contract ("nullptr on failure") is documented.
components/core/tests/test-GlobalMetadataDBConfig.cpp (4)
148-166: Good negative coverage for invalid values.The cases for out-of-range ports and empty required args are clear and aligned with expected validation failures.
Also applies to: 170-198
206-241: SQLite mixed-arg rejection tests are precise and helpful.Overwriting placeholders with MySQL-only flags and asserting validate() throws is a solid way to capture the contract.
243-253: SQLite credential handling assertions are consistent.Verifying that credentials are ignored for sqlite and that validate() succeeds improves confidence in the new env/CLI split.
139-143: No change needed for “Username set but password missing” testThe current implementation in
components/core/src/clp/GlobalMetadataDBConfig.cpp (lines 177–183)
requires bothCLP_DB_USERandCLP_DB_PASSto be set (even if the password is empty). A missingCLP_DB_PASSleads to an exception invalidate(), so the existing test correctly asserts a throw for “username set but password missing.”Likely an incorrect or invalid review comment.
Comment on lines
+115
to
+116
| REQUIRE((config.get_metadata_db_username() == cMysqlDbUser)); | ||
| REQUIRE((config.get_metadata_db_password() == cMysqlDbPass)); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make optional checks explicit to avoid ambiguous comparisons.
These assertions compare a std::optional to a string literal, which relies on library/operator support and may be brittle across compilers. Prefer explicit has_value() and value() checks.
- REQUIRE((config.get_metadata_db_username() == cMysqlDbUser));
- REQUIRE((config.get_metadata_db_password() == cMysqlDbPass));
+ REQUIRE(true == config.get_metadata_db_username().has_value());
+ REQUIRE(config.get_metadata_db_username().value() == cMysqlDbUser);
+ REQUIRE(true == config.get_metadata_db_password().has_value());
+ REQUIRE(config.get_metadata_db_password().value() == cMysqlDbPass);- REQUIRE((config.get_metadata_db_username() == cMysqlDbUser));
- REQUIRE((config.get_metadata_db_password() == ""));
+ REQUIRE(true == config.get_metadata_db_username().has_value());
+ REQUIRE(config.get_metadata_db_username().value() == cMysqlDbUser);
+ REQUIRE(true == config.get_metadata_db_password().has_value());
+ REQUIRE(config.get_metadata_db_password().value().empty());Also applies to: 125-126
🤖 Prompt for AI Agents
In components/core/tests/test-GlobalMetadataDBConfig.cpp around lines 115-116
(and similarly lines 125-126), the tests compare std::optional<T> directly to
string literals; change each assertion to first REQUIRE that the optional
has_value() and then REQUIRE that optional.value() == expected string, so the
checks are explicit and avoid relying on implicit/ambiguous operator overloads
across compilers.
junhaoliao
commented
Aug 13, 2025
Member
Author
There was a problem hiding this comment.
I reviewed by checking individual newly added commits then skimmed through all changes in the whole PR.
(though it was me who proposed the original PR title.) is below a better one?
feat(core)!: Replace YAML config with CLI args and env vars for metadata DB (resolves #1146).
| By default, `clp` uses an embedded SQLite database, so each directory containing archives can only | ||
| be accessed by a single `clp` instance. | ||
| To enable parallel compression to the same archives directory, `clp` (and by extension, `clg`) needs | ||
| to be configured to use a MySQL-type database (e.g., MariaDB) rather than the default---an embedded |
Member
Author
There was a problem hiding this comment.
any reason we want to use --- than -? not sure if it's a standard punctuation
Member
There was a problem hiding this comment.
- is used to join two words. --- is used as a kind of interjection.
https://www.grammarly.com/blog/punctuation-capitalization/hyphens-and-dashes/
An em dash is most often used to indicate a pause in a sentence. It’s stronger than a comma, but weaker than a period or semicolon.
Member
Author
There was a problem hiding this comment.
gotcha. i didn't realize it was meant to be an "em dash"
Co-authored-by: Junhao Liao <junhao.liao@yscope.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
docs/src/user-guide/core-unstructured/clp.md (3)
108-110: Limit the requirement to clp (don’t imply clg is part of compression).clg isn’t involved in compression; mentioning it here can confuse readers.
Apply this diff:
-To enable parallel compression to the same archives directory, `clp` (and by extension, `clg`) needs +To enable parallel compression to the same archives directory, `clp` needs
116-116: Clarify that clp and clg must point to the same DB instance.This removes ambiguity when users configure both tools.
-You can configure `clp` and `clg` to use a MySQL-compatible database as follows: +You can configure `clp` and `clg` to use a MySQL-compatible database as follows. Both tools must +point to the same database instance you initialise in the next step:
131-132: Simplify phrasing and add a brief security tip for CLP_DB_PASS.Avoid “user’s username/password” redundancy and help users prevent credential leakage.
- * Set the `CLP_DB_USER` environment variable for the database user's username - * Set the `CLP_DB_PASS` environment variable for the database user's password + * Set the `CLP_DB_USER` environment variable for the database username + * Set the `CLP_DB_PASS` environment variable for the database password + * Tip: Avoid exposing credentials in shell history; prefer reading the password interactively + (e.g., with `read -s`) or using a secrets manager.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/src/user-guide/core-unstructured/clp.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
docs/src/user-guide/core-unstructured/clp.md
🪛 LanguageTool
docs/src/user-guide/core-unstructured/clp.md
[style] ~119-~119: Consider using “who” when you are referring to a person instead of an object.
Context: ...tions for your platform * Create a user that has privileges to create databases, cre...
(THAT_WHO)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (3)
docs/src/user-guide/core-unstructured/clp.md (3)
112-114: LGTM: clear, actionable warning.The warning is concise and accurately describes the SQLite limitation.
121-121: LGTM: clearer punctuation around the install command.The colon after the instruction reads cleanly.
137-138: LGTM: correctly scopes parallelization to clp only.Accurately reflects that only clp supports parallel runs.
Comment on lines
+133
to
+136
| * Run `clp` or `clg` with the same command-line options and environment variables, with the addition | ||
| of the database type command-line option: | ||
| * `--db-type mysql` to specify MySQL as the database type | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add a concrete, copy-paste example for env vars + CLI flags.
A short example reduces setup friction and demonstrates the new flow end-to-end.
* `--db-type mysql` to specify MySQL as the database type
+
+```shell
+# Example setup
+export CLP_DB_USER=clpuser
+read -s -p "DB password: " CLP_DB_PASS && echo
+export CLP_DB_PASS
+
+# Initialize the DB schema (adjust as needed)
+./tools/scripts/db/init-db.py --db-host 127.0.0.1 --db-port 3306 --db-name clp-db --db-table-prefix clp_
+
+# Run clp using a MySQL-compatible database
+./clp c --db-type mysql /path/to/archives /path/to/logs
+```🤖 Prompt for AI Agents
In docs/src/user-guide/core-unstructured/clp.md around lines 133 to 136, the doc
mentions running clp/clg with environment variables and CLI options but lacks a
concrete, copy-paste example; add a short shell example showing exporting
CLP_DB_USER and CLP_DB_PASS (prompting for password), running the DB
initialization script with host/port/name/table-prefix, and then invoking clp
with --db-type mysql and the archive/log paths so users can copy-paste an
end-to-end setup and run sequence.
junhaoliao
commented
Aug 13, 2025
junhaoliao
left a comment
Member
Author
There was a problem hiding this comment.
with the proposed fix, i validated by
- Started CLP Text package.
- spawned multiple compression requests at the same time
./compress.sh ~/samples/hive-24hr/ &(roughly 10 requests in 3 seconds) - observed all compression jobs completed successfully
- in the webui, performed search with query string "1" and observed query completed successfully
cd bin; ./clp c test ~/samples/hive-24hr/i-00c90a0f/Then inspected the archive output folder and saw a sqlite db local file.
Similarly, for CLP-JSON package:
- Repeated step 1-5 above. The logs used was
~/samples/postgresql/. the binary name should beclp-sinstead ofclp. cd bin./indexer default <some_archive_dir_in_var_data_archives>: complained that indexer would not work with sqlite (because--db-typeis by defaultsqlite)./indexer default --db-type=mysql <some_archive_dir_in_var_data_archives>: complained thatCLP_DB_USER/CLP_DB_PASSwas missing, until i set them all../indexer default --db-type=mysql <some_archive_dir_in_var_data_archives>with the env var set: inspected the DB tableclp-db.clp_default_column_metadatawhich contains valid data.
Co-authored-by: Junhao Liao <junhao.liao@yscope.com>
Member
Sgtm. |
kirkrodrigues
approved these changes
Aug 13, 2025
kirkrodrigues
left a comment
Member
There was a problem hiding this comment.
...with the title you proposed.
LinZhihao-723
approved these changes
Aug 14, 2025
3 tasks
This was referenced Aug 14, 2025
4 tasks
3 tasks
junhaoliao
commented
Nov 19, 2025
Member
Author
There was a problem hiding this comment.
this file should be added to the lint:py task in taskfiles/lint.yaml
@coderabbitai create an issue to track
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current lint:py task configuration in taskfiles/lint.yaml
cat taskfiles/lint.yaml | grep -A 20 "lint:py" || cat taskfiles/lint.yaml
Length of output: 32852
🏁 Script executed:
#!/bin/bash
# Create GitHub issue to track adding init-db.py to Python linting task
gh issue create \
--repo y-scope/clp \
--title "Add init-db.py to Python linting task" \
--body "## Background
In PR #1148, the file \`components/core/tools/scripts/db/init-db.py\` was modified to remove the YAML config dependency and switch to environment variables and CLI arguments.
This Python script should be added to the \`lint:py\` task in \`taskfiles/lint.yaml\` to ensure it's included in the Python linting workflow.
## Tasks
- [ ] Update the \`lint:py\` task in \`taskfiles/lint.yaml\` to include \`components/core/tools/scripts/db/init-db.py\` or its parent directory
- [ ] Verify the file passes black and ruff checks
## References
- PR: https://github.com/y-scope/clp/pull/1148
- Comment: https://github.com/y-scope/clp/pull/1148#discussion_r2540062254
Requested by: @junhaoliao" \
--assignee junhaoliao
Length of output: 42
@junhaoliao, I've created GitHub issue #1625 to track adding components/core/tools/scripts/db/init-db.py to the lint:py task in taskfiles/lint.yaml. The issue has been assigned to you and includes the background from this PR along with the tasks needed to complete this work.
🧬 Code Graph Analysis Results
File: components/clp-py-utils/clp_py_utils/sql_adapter.py (lines 31-51)
def connect(self):
"""
Checks out a connection from the pool.
:return: On success, a connection from the pool. On error, a DummyCloseableObject.
"""
try:
return self.pool.connect()
except (mariadb.Error, mysql.connector.Error):
# Periodically log the error so long as a connection error hasn't occurred within the
# past interval.
current_time = time.time()
if (
self.time_of_last_exception_log is None
or current_time - self.time_of_last_exception_log > self.error_reporting_interval
):
self.time_of_last_exception_log = current_time
self.logger.exception(
f"Failed to connect to database. Suppressing further connection error logs for "
f"{self.error_reporting_interval} seconds."
)
return DummyCloseableObject()File: components/clp-py-utils/clp_py_utils/initialize-clp-metadata-db.py (lines 31-65)
def main(argv):
args_parser = argparse.ArgumentParser(description="Sets up CLP's metadata tables.")
args_parser.add_argument("--config", required=True, help="Database config file.")
args_parser.add_argument(
"--storage-engine",
type=str,
choices=[engine.value for engine in StorageEngine],
required=True,
help="Storage engine to create tables for.",
)
parsed_args = args_parser.parse_args(argv[1:])
config_file_path = Path(parsed_args.config)
storage_engine = StorageEngine(parsed_args.storage_engine)
try:
database_config = Database.parse_obj(read_yaml_config_file(config_file_path))
if database_config is None:
raise ValueError(f"Database configuration file '{config_file_path}' is empty.")
sql_adapter = SQL_Adapter(database_config)
clp_db_connection_params = database_config.get_clp_connection_params_and_type(True)
table_prefix = clp_db_connection_params["table_prefix"]
with closing(sql_adapter.create_connection(True)) as metadata_db, closing(
metadata_db.cursor(dictionary=True)
) as metadata_db_cursor:
if StorageEngine.CLP_S == storage_engine:
create_datasets_table(metadata_db_cursor, table_prefix)
else:
create_metadata_db_tables(metadata_db_cursor, table_prefix)
metadata_db.commit()
except:
logger.exception("Failed to create clp metadata tables.")
return -1
return 0🧠 Learnings used
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1501
File: tools/deployment/presto-clp/scripts/init.py:10-13
Timestamp: 2025-10-27T07:07:37.901Z
Learning: In `tools/deployment/presto-clp/scripts/init.py`, the `DATABASE_COMPONENT_NAME` and `DATABASE_DEFAULT_PORT` constants are intentionally duplicated from `clp_py_utils.clp_config` because `clp_py_utils` is not installed in the Presto init script's runtime environment. The two flows are separate and this duplication is documented. There are plans to merge these flows after a future release.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
2 tasks
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
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.
Description
clp binaries
(GLT was not modified; if so, we can also remove the yaml-cpp dependency from the installation scripts.)
compression_worker.py
Configuration template
init-db.py, remove support for YAML configuration files for database credentialsChecklist
breaking change.
I marked this as a breaking change because the clp binaries now no longer accepts db connection parameters via a file. Particularly, in the
clp/ clp-s indexer binaries, this PR--db-config-file--db-typewith default valuesqlite--db-hostwith default value127.0.0.1(orlocalhostwhich is pending discussion at https://github.com/y-scope/clp/pull/1148/files#r2246595284)--db-namewith default valueclp-db--db-table-prefixwith default valueclp_--db-type=mysql, expect environment variables:CLP_DB_USERCLP_DB_PASSValidation performed
CLP_DB_USER=clp-user CLP_DB_PASS=xxx clp c test/ ~/samples/hive-24hr; observed the archive directory was generated without error in the command output../compress.sh ~/samples/hive-24hr/ &and observed all jobs ended with success.Summary by CodeRabbit
New Features
Documentation
Chores
Tests