feat(clp-package): Add API Server config interface and Docker Compose orchestration; Set HOME=/tmp during image build in GH workflow.#1575
Conversation
WalkthroughAdds a Rust API server binary and runtime support: CLI args for config/host/port, environment-based DB credentials, non-blocking file+stdout logging, async graceful shutdown; introduces API server config models in Python and Rust; and updates build tasks, Dockerfiles, Docker Compose services, docs, and CI/taskfile. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(247,251,255,0.95)
participant Controller as Python Controller
participant Deployer as Docker Compose
participant API as API Server (Rust)
participant CFG as /etc/clp-config.yml
participant DB as Database
end
Controller->>Deployer: provide env (CLP_API_SERVER_HOST, CLP_API_SERVER_PORT, CLP_LOGS_DIR, CLP_DB_*)
Deployer->>Deployer: start api-server after init services complete
Deployer->>API: exec /opt/clp/bin/api_server --host --port --config /etc/clp-config.yml
API->>CFG: load config file
API->>API: apply CLI overrides (host/port) and read env vars
API->>DB: connect using env credentials
API->>API: init non-blocking file+stdout logging
API->>API: serve requests
API-->>API: graceful shutdown on SIGTERM or Ctrl+C
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| query_worker["query-worker"] | ||
| reducer["reducer"] | ||
| api_server["api-server"] | ||
| garbage_collector["garbage-collector"] |
There was a problem hiding this comment.
This change is to make the nodes in Management & UI section follow the order api_server, garbage_collector, webui. api_server and garbage_collector are management nodes.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
taskfile.yaml (1)
200-207: Addgeneratesfield to rust task for proper Taskfile dependency tracking.Verification confirms the rust task (lines 200-207) is missing the
generatesfield, which is used throughout taskfile.yaml for dependency caching. The workspace builds a single binary:api_server. The suggested fix withgenerates: ["{{.G_RUST_BUILD_DIR}}/release/api_server"]is accurate and follows the Rust cargo build output conventions.Apply the provided diff to declare the generated binary output.
tools/docker-images/clp-package/Dockerfile (1)
44-44: Consider alphabetizing COPY statements for maintainability.As noted in the past review comment, since all COPY statements use
--link, they can be reordered without affecting the build. Alphabetizing the binary COPY statements would improve maintainability and make it easier to locate specific entries.Consider this alphabetical ordering:
COPY --link --chown=${UID} ./components/package-template/src/ . +COPY --link --chown=${UID} ./build/rust/release/api_server bin/ COPY --link --chown=${UID} ./build/core/clg bin/ COPY --link --chown=${UID} ./build/core/clo bin/ COPY --link --chown=${UID} ./build/core/clp bin/ COPY --link --chown=${UID} ./build/core/clp-s bin/ COPY --link --chown=${UID} ./build/core/indexer bin/ COPY --link --chown=${UID} ./build/core/log-converter bin/ COPY --link --chown=${UID} ./build/core/reducer-server bin/ COPY --link --chown=${UID} ./build/deps/cpp/mariadb-connector-cpp-install/lib/*/libmariadbcpp.so* \ lib/ COPY --link --chown=${UID} ./build/spider/spider-build/src/spider/spider_scheduler bin/ COPY --link --chown=${UID} ./build/spider/spider-build/src/spider/spider_worker bin/ COPY --link --chown=${UID} ./build/nodejs-22/bin/node bin/node-22 COPY --link --chown=${UID} ./build/python-libs/ lib/python3/site-packages/ -COPY --link --chown=${UID} ./build/rust/release/api_server bin/ COPY --link --chown=${UID} ./build/webui/ var/www/webui/tools/deployment/package/docker-compose-all.yaml (1)
497-497: Make RUST_LOG configurable and follow Rust conventions.The
RUST_LOGenvironment variable is hardcoded to"DEBUG"which produces very verbose logs and doesn't follow Rust conventions. Per the env_logger documentation, log level names should be lowercase, and the level should be configurable for different environments.Apply this diff:
CLP_LOGS_DIR: "/var/log/api_server" CLP_DB_PASS: "${CLP_DB_PASS:?Please set a value.}" CLP_DB_USER: "${CLP_DB_USER:?Please set a value.}" - RUST_LOG: "DEBUG" + RUST_LOG: "${CLP_API_SERVER_RUST_LOG:-info}"Then update the controller to set the default in
.envif needed.components/clp-package-utils/clp_package_utils/controller.py (1)
772-775: Consider reordering API server setup for consistency.The API server environment setup is called after webui setup, but the deployment documentation (docs/src/user-docs/guides-multi-host.md) and architecture diagram show the API server starting before webui. While the order may not affect functionality since all environment variables are merged, consider moving line 775 before line 772 for consistency with the documented startup sequence.
Apply this diff to improve consistency:
env_vars |= self._set_up_env_for_compression_worker(num_workers) env_vars |= self._set_up_env_for_query_worker(num_workers) env_vars |= self._set_up_env_for_reducer(num_workers) + env_vars |= self._set_up_env_for_api_server() env_vars |= self._set_up_env_for_webui(container_clp_config) env_vars |= self._set_up_env_for_mcp_server() env_vars |= self._set_up_env_for_garbage_collector() - env_vars |= self._set_up_env_for_api_server()components/clp-py-utils/clp_py_utils/clp_config.py (1)
596-607: Consider adding validation and a DEFAULT_PORT constant for consistency.The
ApiServermodel follows most component patterns but could be improved:
- Validation:
default_max_num_query_resultsshould usePositiveIntinstead of plainintto ensure it's greater than zero.- Consistency: Add a
DEFAULT_PORT: ClassVar[int] = 3001like other components (Database, Queue, Redis, etc.) for clarity and maintainability.Apply this diff:
class ApiServer(BaseModel): + DEFAULT_PORT: ClassVar[int] = 3001 + host: DomainStr = "localhost" - port: Port = 3001 + port: Port = DEFAULT_PORT query_job_polling: QueryJobPollingConfig = QueryJobPollingConfig() - default_max_num_query_results: int = 1000 + default_max_num_query_results: PositiveInt = 1000
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
.github/actions/run-on-image/action.yaml(0 hunks).github/workflows/clp-artifact-build.yaml(1 hunks)components/api-server/src/bin/api_server.rs(2 hunks)components/clp-package-utils/clp_package_utils/controller.py(3 hunks)components/clp-py-utils/clp_py_utils/clp_config.py(3 hunks)components/package-template/src/etc/clp-config.yml(1 hunks)docs/src/dev-docs/design-deployment-orchestration.md(4 hunks)docs/src/user-docs/guides-multi-host.md(1 hunks)taskfile.yaml(5 hunks)tools/deployment/package/docker-compose-all.yaml(1 hunks)tools/docker-images/clp-package/Dockerfile(1 hunks)
💤 Files with no reviewable changes (1)
- .github/actions/run-on-image/action.yaml
🧰 Additional context used
🧠 Learnings (25)
📓 Common learnings
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1575
File: components/clp-py-utils/clp_py_utils/clp_config.py:602-607
Timestamp: 2025-11-10T05:19:56.600Z
Learning: In the y-scope/clp repository, the `ApiServer` class in `components/clp-py-utils/clp_py_utils/clp_config.py` does not need a `transform_for_container()` method because no other containerized service depends on the API server - it's only accessed from the host, so no docker-network communication is expected.
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.
📚 Learning: 2025-11-03T16:17:40.223Z
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.
Applied to files:
components/package-template/src/etc/clp-config.ymlcomponents/api-server/src/bin/api_server.rstools/deployment/package/docker-compose-all.yamlcomponents/clp-package-utils/clp_package_utils/controller.pycomponents/clp-py-utils/clp_py_utils/clp_config.py
📚 Learning: 2025-11-10T05:19:56.600Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1575
File: components/clp-py-utils/clp_py_utils/clp_config.py:602-607
Timestamp: 2025-11-10T05:19:56.600Z
Learning: In the y-scope/clp repository, the `ApiServer` class in `components/clp-py-utils/clp_py_utils/clp_config.py` does not need a `transform_for_container()` method because no other containerized service depends on the API server - it's only accessed from the host, so no docker-network communication is expected.
Applied to files:
components/package-template/src/etc/clp-config.ymltools/deployment/package/docker-compose-all.yamlcomponents/clp-package-utils/clp_package_utils/controller.pycomponents/clp-py-utils/clp_py_utils/clp_config.py
📚 Learning: 2025-08-25T16:27:50.549Z
Learnt from: davemarco
Repo: y-scope/clp PR: 1198
File: components/webui/server/src/plugins/app/Presto.ts:38-43
Timestamp: 2025-08-25T16:27:50.549Z
Learning: In the CLP webui Presto configuration, host and port are set via package settings (configurable), while user, catalog, and schema are set via environment variables (environment-specific). This mixed approach is intentional - settings are typically set by package and some values don't need to be package-configurable.
Applied to files:
components/package-template/src/etc/clp-config.ymlcomponents/clp-py-utils/clp_py_utils/clp_config.py
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).
Applied to files:
tools/docker-images/clp-package/Dockerfiletaskfile.yaml.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-10-17T19:59:25.596Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-package-utils/clp_package_utils/controller.py:315-315
Timestamp: 2025-10-17T19:59:25.596Z
Learning: In components/clp-package-utils/clp_package_utils/controller.py, worker log directories (compression_worker, query_worker, reducer) created via `mkdir()` do not need `_chown_paths_if_root()` calls because directories are created with the same owner as the script caller. This differs from infrastructure service directories (database, queue, Redis, results cache) which do require explicit ownership changes.
Applied to files:
components/clp-package-utils/clp_package_utils/controller.py
📚 Learning: 2025-10-07T07:54:32.427Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-py-utils/clp_py_utils/clp_config.py:47-47
Timestamp: 2025-10-07T07:54:32.427Z
Learning: In components/clp-py-utils/clp_py_utils/clp_config.py, the CONTAINER_AWS_CONFIG_DIRECTORY constant is intentionally set to pathlib.Path("/") / ".aws" (i.e., `/.aws`) rather than a user-specific home directory. This hardcoded path is part of the container orchestration design.
Applied to files:
components/clp-package-utils/clp_package_utils/controller.py
📚 Learning: 2025-10-27T07:07:37.901Z
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.
Applied to files:
components/clp-package-utils/clp_package_utils/controller.py
📚 Learning: 2025-10-22T21:01:31.391Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:01:31.391Z
Learning: In y-scope/clp, for the clp-rust-checks workflow (GitHub Actions) and the Taskfile target deps:lock:check-rust, we should verify Rust Cargo.lock is in sync with Cargo.toml using a non-mutating method (e.g., cargo metadata --locked / cargo check --locked) to keep CI deterministic and avoid updating dependencies during validation.
Applied to files:
taskfile.yaml
📚 Learning: 2025-05-06T09:46:42.639Z
Learnt from: kirkrodrigues
Repo: y-scope/clp PR: 881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:15-23
Timestamp: 2025-05-06T09:46:42.639Z
Learning: For temporary solutions in installation scripts like those in `components/core/tools/scripts/lib_install/`, checksumming downloaded files is considered optional, particularly when working with trusted sources like GitHub raw content.
Applied to files:
taskfile.yaml
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 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:
taskfile.yaml
📚 Learning: 2025-08-25T00:45:05.464Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1242
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:39-41
Timestamp: 2025-08-25T00:45:05.464Z
Learning: Task v3.44.1 causes issues in the clp-ffi-js project (issue #110), so CLP should avoid upgrading to v3.44.1 and stick with v3.44.0 for now.
Applied to files:
taskfile.yaml
📚 Learning: 2025-07-23T09:54:45.185Z
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.
Applied to files:
taskfile.yaml.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-10-27T04:05:01.560Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1507
File: taskfiles/deps/lock.yaml:66-76
Timestamp: 2025-10-27T04:05:01.560Z
Learning: In the CLP project, the `uv` CLI tool is documented as a prerequisite in docs/src/dev-docs/building-package.md and is not installed via a task dependency like the rust toolchain. UV is expected to be available in the environment rather than being provisioned by the taskfile system.
Applied to files:
taskfile.yaml
📚 Learning: 2025-10-22T21:14:12.225Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.
Applied to files:
taskfile.yaml.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-09-25T19:26:32.436Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1335
File: taskfiles/docker-images.yaml:15-15
Timestamp: 2025-09-25T19:26:32.436Z
Learning: In the CLP project's Taskfile, the `:package` task creates the G_PACKAGE_BUILD_DIR directory structure, so any task that depends on `:package` (like `docker-images:package`) can safely assume this directory exists without needing additional mkdir commands.
Applied to files:
taskfile.yaml.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-07-25T21:29:48.947Z
Learnt from: Bill-hbrhbr
Repo: y-scope/clp PR: 1126
File: .gitignore:5-5
Timestamp: 2025-07-25T21:29:48.947Z
Learning: In the CLP project, the .clang-format file is maintained in the yscope-dev-utils submodule and copied over to the main CLP repository, so it should be ignored in .gitignore to prevent accidental commits of the copied file and maintain the single source of truth in the submodule.
Applied to files:
taskfile.yaml
📚 Learning: 2025-10-13T03:32:19.293Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1414
File: tools/docker-images/clp-package/Dockerfile:20-24
Timestamp: 2025-10-13T03:32:19.293Z
Learning: In the clp repository's Dockerfiles (e.g., tools/docker-images/clp-package/Dockerfile), ENV directives should be split into separate lines for readability rather than consolidated to reduce layer count. This is especially true for PATH modifications, as agreed upon in PR #1166. Later ENV settings may depend on earlier ones (e.g., referencing CLP_HOME).
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell
Repo: y-scope/clp PR: 918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-10-20T21:05:30.417Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1413
File: tools/docker-images/clp-package/Dockerfile:22-24
Timestamp: 2025-10-20T21:05:30.417Z
Learning: In the clp repository's Dockerfiles, ENV directives should be consolidated into multi-line ENV statements when possible to reduce image layers. ENV statements should only be split into separate commands when consolidation is not possible due to dependencies (e.g., when later variables must reference earlier ones that need to be set first, or when PATH must be modified sequentially).
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-08-09T04:07:27.083Z
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.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-07-07T17:43:04.349Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-07-01T14:51:19.172Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 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:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-07-01T14:52:02.418Z
Learnt from: jackluo923
Repo: y-scope/clp PR: 1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Applied to files:
.github/workflows/clp-artifact-build.yaml
📚 Learning: 2025-05-27T20:04:51.498Z
Learnt from: anlowee
Repo: y-scope/clp PR: 925
File: .github/workflows/clp-s-antlr-generation.yaml:24-27
Timestamp: 2025-05-27T20:04:51.498Z
Learning: The clp codebase uses commit SHAs instead of version tags for GitHub Actions (like actions/checkout) as an established pattern across workflow files.
Applied to files:
.github/workflows/clp-artifact-build.yaml
🧬 Code graph analysis (3)
components/api-server/src/bin/api_server.rs (2)
components/clp-rust-utils/src/serde/yaml.rs (1)
from_path(18-21)components/clp-py-utils/clp_py_utils/clp_config.py (1)
Database(164-251)
components/clp-package-utils/clp_package_utils/controller.py (1)
components/clp-py-utils/clp_py_utils/core.py (1)
resolve_host_path_in_container(83-104)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
components/clp-rust-utils/src/clp_config/package/config.rs (6)
default(21-29)default(48-54)default(67-74)default(88-93)default(110-114)default(142-148)
🪛 Ruff (0.14.4)
components/clp-package-utils/clp_package_utils/controller.py
437-437: Logging statement uses f-string
(G004)
🔇 Additional comments (11)
.github/workflows/clp-artifact-build.yaml (1)
550-553: Verify consistency of HOME environment variable across all run_command invocations.Line 552 adds
HOME=/tmpto the package-image job's run_command, following the pattern established at lines 495–498 in ubuntu-jammy-lint. However, other run_command invocations (lines 233–239, 273–279, 313–319, 356–362) do not set HOME.Consider whether this environment variable should be uniformly applied to all run_command invocations that execute within containers, or if selective application is intentional based on specific container configurations. If selective application is intentional, document why package-image and ubuntu-jammy-lint require HOME=/tmp while the binaries jobs do not.
taskfile.yaml (4)
31-31: LGTM — Build directory variables correctly defined.The
G_RUST_BUILD_DIRandG_WEBUI_BUILD_DIRvariables follow the established pattern and will properly isolate build artefacts.Also applies to: 33-33
76-77: LGTM — clean-rust integrated correctly.The
clean-rusttask is properly placed in the clean task's dependency list, maintaining alphabetical ordering with other cleanup tasks.
100-102: LGTM — clean-rust task definition is correct.The simple
rm -rfcommand properly cleans the Rust build directory.
235-235: LGTM — rust dependency correctly added to package-build-deps.The rust task is now a dependency of the package build pipeline, ensuring the API server binary is built before packaging.
components/package-template/src/etc/clp-config.yml (1)
121-129: LGTM! API server configuration structure is well-defined.The commented-out configuration block follows the established pattern for optional components and provides sensible defaults that align with the Python and Rust configuration models.
docs/src/dev-docs/design-deployment-orchestration.md (1)
47-48: LGTM! Documentation accurately reflects the new API server component.The architecture diagram, dependency graph, and service table have been correctly updated to include the API server. The ordering (api_server before webui) aligns with the startup sequence and addresses the past review feedback.
Also applies to: 67-68, 99-103, 131-131
components/clp-package-utils/clp_package_utils/controller.py (1)
429-451: LGTM! API server environment setup follows established patterns.The
_set_up_env_for_api_servermethod correctly creates the component's logs directory and returns the necessary environment variables (host and port) for Docker Compose deployment.components/api-server/src/bin/api_server.rs (2)
34-54: LGTM! Helper functions improve code organization.The extraction of
read_config_and_credentials,set_up_logging, andshutdown_signalinto separate functions addresses the past review feedback and significantly improves readability. The separation of concerns makes the startup flow inmainmuch clearer.Also applies to: 56-69, 71-80
89-93: LGTM! Address selection logic is clear and flexible.The CLI arguments correctly override configuration file values for host and port, providing deployment flexibility while maintaining sensible defaults from the config file.
tools/deployment/package/docker-compose-all.yaml (1)
490-515: LGTM! API server service definition follows established patterns.The service configuration correctly:
- Sets up dependencies on initialization jobs
- Mounts necessary volumes (config and logs)
- Uses environment variables for database credentials
- Exposes the API server port with configurable host/port via env vars
|
@LinZhihao-723 @junhaoliao ready for another review. Any suggestions on the title? |
junhaoliao
left a comment
There was a problem hiding this comment.
for the title, how about:
feat(clp-package): Add API Server config interface and Docker Compose orchestration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
601-606: Consider addingDEFAULT_PORTClassVar and improving type safety for consistency.The
ApiServerclass is missing patterns used by all other component classes with port fields (e.g.,Database,QueryScheduler,Redis,Reducer,ResultsCache,Queue,WebUi,McpServer):
- Missing
DEFAULT_PORTClassVar: All similar components define this constant for clarity and maintainability.- Type safety:
default_max_num_query_resultsuses plainint, which permits zero or negative values. Consider usingPositiveIntfor validation.Note: The use of
DomainStrandPorttypes is correct and aligns with previous feedback. As per learnings,transform_for_container()is not needed since no other containerised service depends on the API server.Apply this diff to improve consistency:
class ApiServer(BaseModel): + DEFAULT_PORT: ClassVar[int] = 3001 + host: DomainStr = "localhost" - port: Port = 3001 + port: Port = DEFAULT_PORT query_job_polling: QueryJobPollingConfig = QueryJobPollingConfig() - default_max_num_query_results: int = 1000 + default_max_num_query_results: PositiveInt = 1000Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
components/clp-py-utils/clp_py_utils/clp_config.py(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1575
File: components/clp-py-utils/clp_py_utils/clp_config.py:602-607
Timestamp: 2025-11-10T05:19:56.600Z
Learning: In the y-scope/clp repository, the `ApiServer` class in `components/clp-py-utils/clp_py_utils/clp_config.py` does not need a `transform_for_container()` method because no other containerized service depends on the API server - it's only accessed from the host, so no docker-network communication is expected.
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.
📚 Learning: 2025-11-10T05:19:56.600Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1575
File: components/clp-py-utils/clp_py_utils/clp_config.py:602-607
Timestamp: 2025-11-10T05:19:56.600Z
Learning: In the y-scope/clp repository, the `ApiServer` class in `components/clp-py-utils/clp_py_utils/clp_config.py` does not need a `transform_for_container()` method because no other containerized service depends on the API server - it's only accessed from the host, so no docker-network communication is expected.
Applied to files:
components/clp-py-utils/clp_py_utils/clp_config.py
📚 Learning: 2025-11-03T16:17:40.223Z
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.
Applied to files:
components/clp-py-utils/clp_py_utils/clp_config.py
📚 Learning: 2025-10-27T07:07:37.901Z
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.
Applied to files:
components/clp-py-utils/clp_py_utils/clp_config.py
📚 Learning: 2025-08-25T16:27:50.549Z
Learnt from: davemarco
Repo: y-scope/clp PR: 1198
File: components/webui/server/src/plugins/app/Presto.ts:38-43
Timestamp: 2025-08-25T16:27:50.549Z
Learning: In the CLP webui Presto configuration, host and port are set via package settings (configurable), while user, catalog, and schema are set via environment variables (environment-specific). This mixed approach is intentional - settings are typically set by package and some values don't need to be package-configurable.
Applied to files:
components/clp-py-utils/clp_py_utils/clp_config.py
🧬 Code graph analysis (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
components/clp-rust-utils/src/clp_config/package/config.rs (6)
default(21-29)default(48-54)default(67-74)default(88-93)default(110-114)default(142-148)
⏰ 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). (2)
- GitHub Check: manylinux_2_28-x86_64-deps-image
- GitHub Check: rust-checks (ubuntu-22.04)
🔇 Additional comments (2)
components/clp-py-utils/clp_py_utils/clp_config.py (2)
39-39: LGTM!The constant follows the established naming pattern and is appropriately placed with other component name constants.
643-643: LGTM!The
api_serverfield is correctly integrated intoCLPConfig, following the same pattern as other component fields.
| class QueryJobPollingConfig(BaseModel): | ||
| initial_backoff_ms: int = Field(default=100, alias="initial_backoff") | ||
| max_backoff_ms: int = Field(default=5000, alias="max_backoff") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using PositiveInt for type safety.
Both initial_backoff_ms and max_backoff_ms fields use plain int, which permits negative values. Since negative backoff durations are invalid, consider using PositiveInt (defined at line 83) to enforce validation, consistent with other timing fields in this file like jobs_poll_delay and upsert_interval.
Apply this diff to add validation:
class QueryJobPollingConfig(BaseModel):
- initial_backoff_ms: int = Field(default=100, alias="initial_backoff")
- max_backoff_ms: int = Field(default=5000, alias="max_backoff")
+ initial_backoff_ms: PositiveInt = Field(default=100, alias="initial_backoff")
+ max_backoff_ms: PositiveInt = Field(default=5000, alias="max_backoff")📝 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.
| class QueryJobPollingConfig(BaseModel): | |
| initial_backoff_ms: int = Field(default=100, alias="initial_backoff") | |
| max_backoff_ms: int = Field(default=5000, alias="max_backoff") | |
| class QueryJobPollingConfig(BaseModel): | |
| initial_backoff_ms: PositiveInt = Field(default=100, alias="initial_backoff") | |
| max_backoff_ms: PositiveInt = Field(default=5000, alias="max_backoff") |
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/clp_config.py around lines 596 to 598,
the two backoff fields are typed as plain int allowing negatives; change their
type annotations to PositiveInt (the alias defined at line 83) to enforce
positive values while keeping the existing Field(default=..., alias=...)
settings. Update both initial_backoff_ms and max_backoff_ms to use PositiveInt
so validation matches other timing fields.
LinZhihao-723
left a comment
There was a problem hiding this comment.
I think the fix is to add this line at the top of the task file.
| | compression_worker | Worker processes for compression jobs | | ||
| | query_worker | Worker processes for search/aggregation jobs | | ||
| | reducer | Reducers for performing the final stages of aggregation jobs | | ||
| | api_server | API server for submitting queries | |
There was a problem hiding this comment.
just a note - in future PRs, we should update the description here when we add new responsibilities to the API server
|
let's also the CI workflow change in the title. e.g., |
HOME=/tmp during image build in GH workflow.
| "http://mcp_server:8000/health" | ||
| ] | ||
|
|
||
| api-server: |
There was a problem hiding this comment.
sorry for missing this in the code review - a healthcheck should be added to check the liveness of the service, as done in the mcp-server
@coderabbitai create an issue to check
There was a problem hiding this comment.
🧩 Analysis chain
@junhaoliao, I'll create an issue to track adding a healthcheck for the api-server service.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track adding healthcheck for api-server
gh issue create \
--title "Add healthcheck to api-server service in Docker Compose" \
--body "## Description
The \`api-server\` service in the Docker Compose configuration lacks a healthcheck block to verify service liveness, similar to other services like \`mcp-server\`.
## Context
- **PR**: https://github.com/y-scope/clp/pull/1575
- **Comment**: https://github.com/y-scope/clp/pull/1575#discussion_r2531019587
- **File**: \`tools/deployment/package/docker-compose-all.yaml\`
- **Requested by**: @junhaoliao
## Expected Outcome
Add a healthcheck configuration to the \`api-server\` service to ensure proper monitoring of service health status during orchestration." \
--assignee junhaoliao \
--repo y-scope/clp
Length of output: 42
@junhaoliao, I've created issue #1608 to track adding a healthcheck for the api-server service. The issue includes references to this PR and comment as backlinks.
🧠 Learnings used
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1136
File: components/webui/client/src/pages/IngestPage/Details/sql.ts:1-1
Timestamp: 2025-07-29T21:00:07.757Z
Learning: User junhaoliao requested creating a GitHub issue to track server-side SQL query error handling improvements, specifically to prevent uncaught query errors from causing 500 errors to reach the client.
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1575
File: components/clp-py-utils/clp_py_utils/clp_config.py:602-607
Timestamp: 2025-11-10T05:19:56.600Z
Learning: In the y-scope/clp repository, the `ApiServer` class in `components/clp-py-utils/clp_py_utils/clp_config.py` does not need a `transform_for_container()` method because no other containerized service depends on the API server - it's only accessed from the host, so no docker-network communication is expected.
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.
… orchestration; Set `HOME=/tmp` during image build in GH workflow. (y-scope#1575)
Description
docker-compose.*.yamlcontroller.pyChecklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores