refactor(deployment): Refactor Package Docker Compose files using service-level compositions.#1522
Conversation
WalkthroughA refactoring of the Docker Compose configuration structure introduces a hierarchical composition pattern. A new helper method in the controller selects between deployment-type-specific compose files. Service definitions are centralized in a new docker-compose-all.yaml file, with docker-compose-base.yaml and docker-compose.yaml extending from it. Four new services (query-scheduler, query-worker, reducer, mcp-server) are added alongside a shared utilities file for common volumes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-27T07:07:37.901ZApplied 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). (3)
🔇 Additional comments (3)
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 |
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 (4)
components/clp-package-utils/clp_package_utils/controller.py(2 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)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-25T16:27:50.549Z
Learnt from: davemarco
PR: y-scope/clp#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:
tools/deployment/package/docker-compose-base.yaml
📚 Learning: 2025-10-27T07:07:37.901Z
Learnt from: junhaoliao
PR: y-scope/clp#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:
tools/deployment/package/docker-compose-base.yamltools/deployment/package/docker-compose.yaml
🧬 Code graph analysis (1)
components/clp-package-utils/clp_package_utils/controller.py (1)
components/clp-py-utils/clp_py_utils/clp_config.py (2)
get_deployment_type(777-781)DeploymentType(90-92)
⏰ 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). (1)
- GitHub Check: package-image
🔇 Additional comments (9)
components/clp-package-utils/clp_package_utils/controller.py (2)
797-804: LGTM! Clean separation of compose file selection logic.The method correctly maps deployment types to their respective compose files, making the logic explicit and maintainable.
746-746: LGTM! Proper integration of dynamic file selection.The change correctly integrates the new method to select the appropriate compose file based on deployment type.
tools/deployment/package/docker-compose-all.yaml (5)
362-399: LGTM! Query scheduler service is well-configured.The service definition follows established patterns from the compression-scheduler, with appropriate dependencies, environment variables, and health checks.
401-433: LGTM! Query worker configuration mirrors compression worker pattern.The service appropriately adapts the compression-worker pattern for query workloads, with correct volume mounts for archive and stream access.
435-458: LGTM! Reducer service is properly configured.The service correctly depends on the query-scheduler being healthy and has appropriate volume mounts for its operational needs.
460-496: LGTM! MCP server service follows established patterns.The service is properly configured with conditional deployment via the "mcp" profile, matching the controller logic at line 747-748 of controller.py.
489-496: No issues found – curl is installed in the container image.Verification confirms that
curlis explicitly installed in theclp-packagecontainer image via the setup script (./tools/docker-images/clp-package/setup-scripts/install-prebuilt-packages.sh). The healthcheck at lines 489–496 will function correctly.tools/deployment/package/docker-compose.yaml (1)
12-80: LGTM! Extends pattern correctly eliminates service definition duplication.The use of
extendsto reference service definitions from docker-compose-all.yaml follows Docker Compose best practices and successfully removes redundant configuration.tools/deployment/package/docker-compose-base.yaml (1)
1-60: LGTM! Base deployment configuration is correct.The package name
clp-package-prestois appropriate for BASE deployment (Presto), and the service selection correctly excludes query-related services that are not needed for Presto-based deployments. The extends pattern successfully reduces duplication while maintaining deployment-specific service composition.
There was a problem hiding this comment.
for these services, can we label with a profile in the "all.yaml"? like profiles: ["base"]
Then we don't need this "base.yaml". To launch bases services, we can just run docker compose -f docker-compose.all.yaml --profile base up
We should still keep the docker-compose.yaml file, there we reset the profiles of the services with the !reset [] syntax, so we can just use docker compose up to launch the default set of services
There was a problem hiding this comment.
the reset syntax: https://docs.docker.com/reference/compose-file/merge/#reset-value
There was a problem hiding this comment.
The whole point of this PR is to make the docker compose file easily extensible. Adding a base profile does not really help if we later introduces a new "profile". Then we need to introduce another profile, and maybe a new docker file as well with resets, making it hard to track how the variables are defined, and what docker compose file to use.
Using the extends approach, for each profile we have a single file listing all the services it needs and the override variables. This way is more structured than using the profile.
There was a problem hiding this comment.
right, that makes a lot of sense
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 (4)
tools/deployment/package/docker-compose-all.yaml(2 hunks)tools/deployment/package/docker-compose-base.yaml(1 hunks)tools/deployment/package/docker-compose.volume.yaml(1 hunks)tools/deployment/package/docker-compose.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T07:07:37.901Z
Learnt from: junhaoliao
PR: y-scope/clp#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:
tools/deployment/package/docker-compose.yaml
⏰ 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). (1)
- GitHub Check: package-image
junhaoliao
left a comment
There was a problem hiding this comment.
the changes lgtm. i posted some stylistic comments
for the title, would this be better?
refactor(deployment): Refactor Package Docker Compose files using service-level compositions.
There was a problem hiding this comment.
right, that makes a lot of sense
junhaoliao
left a comment
There was a problem hiding this comment.
(let's first see if the stylistic comments make sense and address as needed
Co-authored-by: Junhao Liao <junhao@junhao.ca>
extends to remove redundant service definition.…vice-level compositions. (y-scope#1522) Co-authored-by: Junhao Liao <junhao@junhao.ca>
Description
This PR improves docker compose file extensibility by:
docker-compose-all.yaml.extendsto include default services in the docker compose files.This PR also includes minor improvements:
docker-compose.base.yamltodocker-compose-base.yaml.controller.pyto find the docker file name.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Refactor