feat(api): Add docker compose service for API server. #1575
Conversation
WalkthroughAdds a Rust API server binary and build/packaging support, new API server config models and defaults, Python deployment env wiring, Docker Compose service entries, Dockerfile copy of the binary, and Taskfile tasks to build/clean Rust artifacts. Changes
Sequence Diagram(s)sequenceDiagram
rect `#f7fbff`
participant Deployer as Docker Compose
participant Ctrl as Python Controller
participant API as API Server (Rust)
participant CFG as /etc/clp-config.yml
participant DB as Database
end
Deployer->>Ctrl: start deployment
Ctrl->>Ctrl: _set_up_env_for_api_server()
Ctrl->>Deployer: supply env (CLP_API_SERVER_HOST, CLP_API_SERVER_PORT, CLP_LOGS_DIR, CLP_DB_*)
Deployer->>Deployer: start api-server (wait depends_on)
Deployer->>API: exec /opt/clp/bin/api_server --host --port --config /etc/clp-config.yml
API->>CFG: load config (merge CLI overrides)
API->>API: read env CLP_DB_USER, CLP_DB_PASS, CLP_LOGS_DIR
API->>DB: connect using credentials
API->>API: init logging (stdout + non-blocking file)
API->>API: bind to host:port and serve
API-->>API: graceful shutdown on SIGTERM or Ctrl+C
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
| dir: "{{.ROOT_DIR}}" | ||
| cmd: |- | ||
| . "$HOME/.cargo/env" | ||
| cargo build --release --bins --target-dir {{.G_RUST_BUILD_DIR}} |
There was a problem hiding this comment.
cargo does an excellent job in detecting changes in both sources and output binary. We don't need to do checksum here.
There was a problem hiding this comment.
shall we also document this inline for future developers?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
components/api-server/Cargo.toml(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/clp-rust-utils/src/clp_config/package/config.rs(1 hunks)components/package-template/src/etc/clp-config.yml(1 hunks)taskfile.yaml(5 hunks)tools/deployment/package/docker-compose-all.yaml(1 hunks)tools/deployment/package/docker-compose-base.yaml(1 hunks)tools/deployment/package/docker-compose.yaml(1 hunks)tools/docker-images/clp-package/Dockerfile(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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.
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).
📚 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
📚 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-rust-utils/src/clp_config/package/config.rscomponents/clp-py-utils/clp_py_utils/clp_config.pytools/deployment/package/docker-compose-all.yamlcomponents/clp-package-utils/clp_package_utils/controller.pycomponents/api-server/src/bin/api_server.rscomponents/package-template/src/etc/clp-config.yml
📚 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.pycomponents/package-template/src/etc/clp-config.yml
📚 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-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
🧬 Code graph analysis (3)
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)
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/api-server/src/bin/api_server.rs (1)
components/clp-rust-utils/src/serde/yaml.rs (1)
from_path(18-21)
🪛 Checkov (3.2.334)
tools/docker-images/clp-package/Dockerfile
[low] 1-45: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🪛 Ruff (0.14.3)
components/clp-package-utils/clp_package_utils/controller.py
631-631: Logging statement uses f-string
(G004)
⏰ 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: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: build-macos (macos-15, true)
🔇 Additional comments (20)
components/clp-rust-utils/src/clp_config/package/config.rs (1)
78-78: LGTM: Consistent with existing configuration pattern.The addition of
#[serde(default)]toQueryJobPollingConfigaligns with the existing pattern used for other configuration structs in this file and enables robust deserialization with default values when fields are missing.components/api-server/Cargo.toml (2)
23-23: LGTM: Appropriate dependency for handling sensitive data.The
secrecycrate is a suitable choice for protecting sensitive configuration values such as database passwords.
31-31: LGTM: Extended logging capabilities.The addition of
fmtandstdfeatures totracing-subscriberenables formatted logging output, which is useful for development and debugging.taskfile.yaml (3)
33-33: LGTM: Consistent variable naming.The
G_RUST_BUILD_DIRvariable follows the established naming convention for build directories.
108-110: LGTM: Clean task follows established pattern.The
clean-rusttask is consistent with other component cleanup tasks in the file.
234-234: LGTM: Rust added as package build dependency.Correctly ensures the Rust API server binary is built before the package is assembled.
tools/docker-images/clp-package/Dockerfile (1)
45-45: LGTM: API server binary copied into container image.The copy command follows the established pattern for other binaries in this Dockerfile and correctly references the Rust build output directory.
tools/deployment/package/docker-compose-base.yaml (1)
57-60: LGTM: API server service added to base deployment.The service definition follows the established pattern of extending from
docker-compose-all.yamland is consistent with other services in this file.tools/deployment/package/docker-compose.yaml (1)
77-80: LGTM: API server service added to full deployment.The service definition is consistent with the base deployment configuration and follows the established pattern.
components/clp-package-utils/clp_package_utils/controller.py (3)
14-14: LGTM: Import added for API server component name.The import is appropriately placed with other component name constants.
623-645: LGTM: API server environment setup follows established pattern.The
_set_up_env_for_api_servermethod correctly follows the pattern established by other worker components (compression_scheduler, query_scheduler, etc.) by creating the logs directory and returning connection configuration environment variables. The absence of_chown_paths_if_rootis appropriate, as worker log directories do not require explicit ownership changes.Based on learnings.
775-775: LGTM: API server environment integrated into setup.The call to
_set_up_env_for_api_serveris correctly placed alongside other component setup calls.components/clp-py-utils/clp_py_utils/clp_config.py (3)
42-42: LGTM!The constant follows the established naming convention for component names.
596-600: LGTM with note on field aliases.The field names include
_mssuffix for clarity (e.g.,initial_backoff_ms), while the aliases used in YAML configuration omit this suffix (e.g.,initial_backoff). This design allows cleaner YAML keys while maintaining explicit units in Python code.
644-644: LGTM!The field addition follows the established pattern for component configuration.
components/api-server/src/bin/api_server.rs (5)
23-32: LGTM!The CLI argument structure is well-designed, with required
--configand optional--host/--portoverrides.
36-42: LGTM!Config loading properly uses the CLI-provided path with clear error context.
44-54: LGTM! Excellent logging setup for containerized deployment.The use of
Teeto write logs to both stdout and a file is particularly well-suited for Docker, enabling bothdocker logsaccess and persistent file-based logging.
56-71: LGTM! Secure and flexible configuration.The credentials loading uses environment variables with
SecretStringfor the password (security best practice), and the address binding properly falls back from CLI arguments to configuration values.
90-99: LGTM! Proper graceful shutdown for both Docker and local development.The use of
tokio::select!to handle both SIGTERM (Docker) and Ctrl+C (local) ensures graceful shutdown in different deployment contexts.
| class ApiServer(BaseModel): | ||
| host: str = "localhost" | ||
| port: int = 3001 | ||
| query_job_polling: QueryJobPollingConfig = QueryJobPollingConfig() | ||
| default_max_num_query_results: int = 1000 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider adding consistency features to align with other component classes.
The ApiServer class is missing several patterns used by similar component classes (e.g., Database, Queue, Redis):
- Type safety:
hostshould useDomainStrandportshould usePortfor validation - Class constant: Missing
DEFAULT_PORTClassVar for clarity - Container transformation: Missing
transform_for_container()method
Additionally, CLPConfig.transform_for_container() (lines 847-869) does not transform the api_server field, unlike other components.
Apply this diff to add consistency:
class ApiServer(BaseModel):
- host: str = "localhost"
- port: int = 3001
+ DEFAULT_PORT: ClassVar[int] = 3001
+
+ host: DomainStr = "localhost"
+ port: Port = DEFAULT_PORT
query_job_polling: QueryJobPollingConfig = QueryJobPollingConfig()
default_max_num_query_results: int = 1000
+
+ def transform_for_container(self):
+ self.host = API_SERVER_COMPONENT_NAME
+ self.port = self.DEFAULT_PORTThen add this line in CLPConfig.transform_for_container():
self.reducer.transform_for_container()
+ # Note: API server doesn't need container transformation as it's not accessed from other containers🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/clp_config.py around lines 602-607, the
ApiServer model is inconsistent with other component classes: change host type
to DomainStr and port type to Port, add a ClassVar DEFAULT_PORT for clarity,
implement a transform_for_container(self) method mirroring the pattern used by
Database/Queue/Redis to perform any container-specific adjustments (even if it's
a no-op, include the method for consistency), and update
CLPConfig.transform_for_container() (around lines 847-869) to include the
provided comment line after self.reducer.transform_for_container() indicating
the API server does not require container transformation. Ensure types are
imported if missing and follow the same method signature and behavior pattern
used by other component classes.
There was a problem hiding this comment.
i dont think the transform_for_container() is needed because no other containerized service depends on the api-server at the moment - no docker-network communication is expected
There was a problem hiding this comment.
@junhaoliao, that's a valid point! If the API server is only accessed from the host and not by other containerized services, then transform_for_container() is indeed unnecessary.
Would you still like to apply the other consistency improvements for type safety and clarity?
- Using
DomainStrfor thehostfield (provides validation) - Using
Portfor theportfield (validates port is between 1-65535) - Adding
DEFAULT_PORT: ClassVar[int] = 3001for consistency with other components
These would make the ApiServer model more consistent with similar component classes in the file, even without the container transformation. Let me know your preference!
✏️ Learnings added
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.582Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
| api-server: | ||
| <<: *service_defaults | ||
| hostname: "api_server" | ||
| environment: | ||
| 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: "TRACE" | ||
| ports: | ||
| - host_ip: "${CLP_API_SERVER_HOST:-127.0.0.1}" | ||
| published: "${CLP_API_SERVER_PORT:-3001}" | ||
| target: 3001 | ||
| volumes: | ||
| - *volume_clp_config_readonly | ||
| - *volume_clp_logs | ||
| depends_on: | ||
| db-table-creator: | ||
| condition: "service_completed_successfully" | ||
| results-cache-indices-creator: | ||
| condition: "service_completed_successfully" | ||
| command: [ | ||
| "/opt/clp/bin/api_server", | ||
| "--host", "0.0.0.0", | ||
| "--port", "3001", | ||
| "--config", "/etc/clp-config.yml" | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making RUST_LOG level configurable.
The API server service configuration looks well-structured and follows the established patterns. However, RUST_LOG is hardcoded to "TRACE", which produces very verbose logs.
Consider making this configurable like other services:
environment:
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: "TRACE"
+ RUST_LOG: "${CLP_API_SERVER_LOGGING_LEVEL:-INFO}"📝 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.
| api-server: | |
| <<: *service_defaults | |
| hostname: "api_server" | |
| environment: | |
| 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: "TRACE" | |
| ports: | |
| - host_ip: "${CLP_API_SERVER_HOST:-127.0.0.1}" | |
| published: "${CLP_API_SERVER_PORT:-3001}" | |
| target: 3001 | |
| volumes: | |
| - *volume_clp_config_readonly | |
| - *volume_clp_logs | |
| depends_on: | |
| db-table-creator: | |
| condition: "service_completed_successfully" | |
| results-cache-indices-creator: | |
| condition: "service_completed_successfully" | |
| command: [ | |
| "/opt/clp/bin/api_server", | |
| "--host", "0.0.0.0", | |
| "--port", "3001", | |
| "--config", "/etc/clp-config.yml" | |
| ] | |
| api-server: | |
| <<: *service_defaults | |
| hostname: "api_server" | |
| environment: | |
| 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: "${CLP_API_SERVER_LOGGING_LEVEL:-INFO}" | |
| ports: | |
| - host_ip: "${CLP_API_SERVER_HOST:-127.0.0.1}" | |
| published: "${CLP_API_SERVER_PORT:-3001}" | |
| target: 3001 | |
| volumes: | |
| - *volume_clp_config_readonly | |
| - *volume_clp_logs | |
| depends_on: | |
| db-table-creator: | |
| condition: "service_completed_successfully" | |
| results-cache-indices-creator: | |
| condition: "service_completed_successfully" | |
| command: [ | |
| "/opt/clp/bin/api_server", | |
| "--host", "0.0.0.0", | |
| "--port", "3001", | |
| "--config", "/etc/clp-config.yml" | |
| ] |
🤖 Prompt for AI Agents
In tools/deployment/package/docker-compose-all.yaml around lines 490 to 515,
RUST_LOG is hardcoded to "TRACE"; change it to read from an environment variable
with a sensible default (e.g., CLP_RUST_LOG or CLP_API_SERVER_RUST_LOG) so
operators can control log verbosity without editing the compose file. Update the
api-server environment entry to reference the new variable with a default (use
${VAR:-DEFAULT}) and add a line to the project .env or deployment documentation
describing the new variable and recommended default level (e.g., INFO) for
production.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
thanks for coming up with the changes quickly
please see the suggestions about documentation and code organization. the stylistic suggestions are mostly nitpicky
| 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: "TRACE" |
There was a problem hiding this comment.
would info or debug be more appropriate?
| 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: "TRACE" |
There was a problem hiding this comment.
| 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/webui/ var/www/webui/ | ||
| COPY --link --chown=${UID} ./build/rust/release/api_server bin/ |
There was a problem hiding this comment.
(i know i'm the original author of this source file.. sorry for missing that previously)
now that this has become a long list, shall we alphabetize the COPYs? i believe the effect will be the same because we have specified --link
| host: str = "localhost" | ||
| port: int = 3001 |
There was a problem hiding this comment.
| host: str = "localhost" | |
| port: int = 3001 | |
| host: DomainStr = "localhost" | |
| port: Port = 3001 |
| class ApiServer(BaseModel): | ||
| host: str = "localhost" | ||
| port: int = 3001 | ||
| query_job_polling: QueryJobPollingConfig = QueryJobPollingConfig() | ||
| default_max_num_query_results: int = 1000 | ||
|
|
There was a problem hiding this comment.
i dont think the transform_for_container() is needed because no other containerized service depends on the api-server at the moment - no docker-network communication is expected
There was a problem hiding this comment.
the main function seems a bit long now. would extracting helpers improve readability? e.g.,
async fn read_config_and_credentials()
fn set_up_logging()
// extracted from with_graceful_shutdown
async fn shutdown_signal()
|
|
||
| return env_vars | ||
|
|
||
| def _set_up_env_for_api_server(self) -> EnvVarsDict: |
There was a problem hiding this comment.
sorry for not starting the discussion before you implement -
since both the MCP server and the garbage collector are optionally launched, we previously list them after the "always" launched services. (that - skipping the query services when Presto is enabled - is tricky but another story)
can we list the api-server before the webui? ditto in other files
| "http://mcp_server:8000/health" | ||
| ] | ||
|
|
||
| api-server: |
There was a problem hiding this comment.
let's also update user-docs/guides-multi-host and dev-docs/design-deployment-orchestration
| - "rm -rf '{{.G_WEBUI_SRC_DIR}}/server/node_modules'" | ||
| - "rm -rf '{{.G_WEBUI_SRC_DIR}}/yscope-log-viewer/node_modules'" | ||
|
|
||
| clean-rust: |
There was a problem hiding this comment.
i don't remember why we placed the clp-json-pkg-tar and clp-text-pkg-tar in between the "clean" tasks though likely we shouldn't. still let's place this directly above clean-webui (to also match the proposed order in the clean task), so the diff may look nicer when we feel itchy to clean up the order of the pkg tasks
2 participants
Description
docker-compose.*.yamlcontroller.pyChecklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Improvements
Chores