-
Notifications
You must be signed in to change notification settings - Fork 91
feat(presto-clp): Add Docker compose setup for Presto cluster that can connect to clp-json. #1132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
3a84840
9574059
8dd8795
a8af40c
2015a4a
b78682f
1247bba
1400a90
91040d1
9ec648c
4097478
b1ce135
70a56d7
99ac4e1
6dde297
b22746d
708756d
2eeda5c
8bb98f8
9fc487d
64e8edf
00bd3f1
28f5b70
cccf9fe
04f3d2a
3381eb8
bea5bb6
1f68965
94b0210
22c53e0
2f0de28
d621bf3
7300d20
4bc2f58
80c41d4
9d2146b
ff06c62
cf11694
2618f13
782b87d
8107518
4d896e5
713670a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| # Coordinator common | ||
| PRESTO_COORDINATOR_HTTPPORT="8080" | ||
| PRESTO_COORDINATOR_SERVICENAME="presto-coordinator" | ||
|
|
||
| # Coordinator clp.properties | ||
| PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_PROVIDERTYPE="mysql" | ||
| PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_URL="jdbc:mysql://localhost:6001" | ||
| PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_NAME="clp-db" | ||
| PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_USER="clp-user" | ||
| PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_PASSWORD="123456" | ||
| PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_TABLEPREFIX="clp_" | ||
| PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_SPLITPROVIDER="mysql" | ||
|
|
||
| # Coordinator config.properties | ||
| PRESTO_COORDINATOR_CONFIG_CONFIGPROPERTIES_QUERY_MAXMEMORY="1GB" | ||
| PRESTO_COORDINATOR_CONFIG_CONFIGPROPERTIES_QUERY_MAXMEMORYPERNODE="1GB" | ||
|
|
||
| # Coordinator jvm.config | ||
| PRESTO_COORDINATOR_CONFIG_JVMCONFIG_MAXHEAPSIZE="4G" | ||
| PRESTO_COORDINATOR_CONFIG_JVMCONFIG_G1HEAPREGIONSIZE="32M" | ||
|
|
||
| # Coordinator log.properties | ||
| PRESTO_COORDINATOR_CONFIG_LOGPROPERTIES_LEVEL="DEBUG" | ||
|
|
||
| # Coordinator node.properties | ||
| PRESTO_COORDINATOR_CONFIG_NODEPROPERTIES_ENVIRONMENT="production" | ||
|
|
||
| # Worker common | ||
| PRESTO_WORKER_HTTPPORT="8080" | ||
|
|
||
| # Worker node.properties | ||
| PRESTO_WORKER_CONFIG_NODEPROPERTIES_LOCATION="worker-location" | ||
|
|
||
| # CLP package archives | ||
| CLP_PACKAGE_ARCHIVES=REPLACE_ME | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| # Setup local docker stack for presto + clp | ||
|
|
||
| ## Install docker | ||
|
|
||
| Follow the guide here: [docker] | ||
|
|
||
| # Launch clp-package | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Convert secondary titles to H2 and fix punctuation for Markdown lint-cleanliness The document currently contains five -# Launch clp-package
+# ## Launch clp-package
…
-# Create Docker Cluster
+# ## Create Docker Cluster
…
-# Use cli:
+# ## Use CLI
…
-# Delete docker Cluster
+# ## Delete Docker ClusterAlso applies to: 35-41, 48-57, 65-69 🧰 Tools🪛 markdownlint-cli2 (0.17.2)7-7: Multiple top-level headings in the same document (MD025, single-title, single-h1) 🤖 Prompt for AI Agents |
||
|
|
||
| 1. Find the clp-package for test on our official website [clp-json-v0.4.0]. Here is a sample dataset for demo testing: [postgresql dataset]. | ||
|
|
||
| 2. Untar the clp-package and the postgresql dataset. | ||
|
|
||
| 3. Replace the content of `/path/to/clp-json-package/etc/clp-config.yml` with the output of `demo-assets/init.sh generate_sample_clp_config`. | ||
|
|
||
| 4. Launch: | ||
|
|
||
| ```bash | ||
| # You probably want to run a python 3.9 or newer virtual environment | ||
| sbin/start-clp.sh | ||
| ``` | ||
|
|
||
| 5. Compress: | ||
|
|
||
| ```bash | ||
| # You can also use your own dataset | ||
| sbin/compress.sh --timestamp-key 'timestamp' /path/to/postgresql.log | ||
| ``` | ||
|
|
||
| 6. Use the following command to update `.env`: | ||
|
|
||
| ```bash | ||
| demo-assets/init.sh update_metadata_config /path/to/clp-json-package | ||
| ``` | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| # Create Docker Cluster | ||
|
|
||
| Create a local docker stack: | ||
|
|
||
| ```bash | ||
| docker compose up | ||
| ``` | ||
|
|
||
| To create a docker stack with more than 1 worker (e.g., 3 workers): | ||
| ``` | ||
| docker compose up --scale presto-worker=3 | ||
| ``` | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| # Use cli: | ||
|
|
||
| After all containers are in "Started" states (check by `docker ps`): | ||
|
|
||
| ```bash | ||
| # On your host | ||
| docker exec -it compose-presto-coordinator-1 sh | ||
|
|
||
| # In presto-coordinator container | ||
| /opt/presto-cli --catalog clp --schema default --server localhost:8080 | ||
| ``` | ||
|
|
||
| Example query: | ||
| ```sql | ||
| SELECT * FROM default LIMIT 1; | ||
| ``` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Minor: qualify SQL fenced block for syntax highlighting Add -```sql
+```sql
SELECT * FROM default LIMIT 1;In tools/deployment/presto-clp/README.md around lines 61 to 63, the SQL code |
||
|
|
||
| # Delete docker Cluster | ||
|
|
||
| ```bash | ||
| docker compose down | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
| [clp-json-v0.4.0]: https://github.com/y-scope/clp/releases/tag/v0.4.0 | ||
| [docker]: https://docs.docker.com/engine/install | ||
| [postgresql dataset]: https://zenodo.org/records/10516402 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| connector.name=clp | ||
| clp.metadata-provider-type=${PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_PROVIDERTYPE} | ||
| clp.metadata-db-url=${PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_URL} | ||
| clp.metadata-db-name=${PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_NAME} | ||
| clp.metadata-db-user=${PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_USER} | ||
| clp.metadata-db-password=${PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_PASSWORD} | ||
| clp.metadata-table-prefix=${PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_TABLEPREFIX} | ||
| clp.split-provider-type=${PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_SPLITPROVIDER} | ||
| clp.metadata-filter-config=/opt/presto-server/etc/metadata-filter.json | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Several placeholders have no matching definitions – coordinator fails to start.
Add them to 🤖 Prompt for AI Agents |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| coordinator=true | ||
| node-scheduler.include-coordinator=false | ||
| http-server.http.port=${PRESTO_COORDINATOR_HTTPPORT} | ||
| query.max-memory=${PRESTO_COORDINATOR_CONFIG_CONFIGPROPERTIES_QUERY_MAXMEMORY} | ||
| query.max-memory-per-node=${PRESTO_COORDINATOR_CONFIG_CONFIGPROPERTIES_QUERY_MAXMEMORYPERNODE} | ||
| discovery-server.enabled=true | ||
| discovery.uri=http://${PRESTO_COORDINATOR_SERVICENAME}:${PRESTO_COORDINATOR_HTTPPORT} | ||
| optimizer.optimize-hash-generation=false | ||
| regex-library=RE2J | ||
| use-alternative-function-signatures=true | ||
| inline-sql-functions=false | ||
| nested-data-serialization-enabled=false | ||
| native-execution-enabled=true | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| -server | ||
| -Xmx${PRESTO_COORDINATOR_CONFIG_JVMCONFIG_MAXHEAPSIZE} | ||
| -XX:+UseG1GC | ||
| -XX:G1HeapRegionSize=${PRESTO_COORDINATOR_CONFIG_JVMCONFIG_G1HEAPREGIONSIZE} | ||
| -XX:+UseGCOverheadLimit | ||
| -XX:+ExplicitGCInvokesConcurrent | ||
| -XX:+HeapDumpOnOutOfMemoryError | ||
| -XX:+ExitOnOutOfMemoryError | ||
| -Djdk.attach.allowAttachSelf=true | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Add container-aware memory flags & optional G1 tuning
These keep the heap bounded to ~80 % of the container limit without manual tuning. Consider replacing 🤖 Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,2 @@ | ||||||
| com.facebook.presto=${PRESTO_COORDINATOR_CONFIG_LOGPROPERTIES_LEVEL} | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid quoting the log level value in Docker The extra quotes are not a valid log level and Presto will fall back to the default ( -com.facebook.presto=${PRESTO_COORDINATOR_CONFIG_LOGPROPERTIES_LEVEL}
+com.facebook.presto=${PRESTO_COORDINATOR_CONFIG_LOGPROPERTIES_LEVEL:-INFO}and remove surrounding quotes in the 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| { | ||
| } | ||
|
Comment on lines
+1
to
+2
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Provide a minimal example or document expected schema An empty JSON object is syntactically valid, but future maintainers may be unsure what keys are supported. A commented exemplar or pointer to docs beside this file would improve clarity without affecting runtime. 🤖 Prompt for AI Agents
Comment on lines
+1
to
+2
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @anlowee I think we should explain to the user how to configure this for the timestamp field in their logs, right? And also that it may need to be different for each dataset they compress.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we direct them to the related presto-doc section?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not published and is more general than they need, right? We should write a simplified section for them here. |
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,3 @@ | ||||||||||||||||
| node.environment=${PRESTO_COORDINATOR_CONFIG_NODEPROPERTIES_ENVIRONMENT} | ||||||||||||||||
| node.id=${PRESTO_COORDINATOR_SERVICENAME} | ||||||||||||||||
|
|
||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Presto will refuse to start without node.environment=${PRESTO_COORDINATOR_CONFIG_NODEPROPERTIES_ENVIRONMENT}
-node.id=${PRESTO_COORDINATOR_SERVICENAME}
+# A unique ID per container – fall back to HOSTNAME if not supplied
+node.id=${PRESTO_COORDINATOR_NODE_ID:-${HOSTNAME}}
+# Mandatory data directory
+node.data-dir=/opt/presto-server/data📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,19 @@ | ||||||
| #!/bin/sh | ||||||
|
|
||||||
| # Exit on error | ||||||
| set -e | ||||||
|
|
||||||
| PRESTO_CONFIG_DIR="/opt/presto-server/etc" | ||||||
|
|
||||||
| # Substitute environemnt variables in config template | ||||||
| find /configs -type f | while read -r f; do | ||||||
| ( echo "cat <<EOF"; cat $f; echo "EOF" ) | sh > "${PRESTO_CONFIG_DIR}/$(basename "$f")" | ||||||
| done | ||||||
|
|
||||||
| # Setup the config directory hierarchy | ||||||
| rm -rf ${PRESTO_CONFIG_DIR}/catalog | ||||||
| mkdir -p ${PRESTO_CONFIG_DIR}/catalog | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider consistency with the other generate-configs.sh script. This script uses Also fix the quoting issue: -rm -rf ${PRESTO_CONFIG_DIR}/catalog
-mkdir -p ${PRESTO_CONFIG_DIR}/catalog
+rm -rf "${PRESTO_CONFIG_DIR}/catalog"
+mkdir -p "${PRESTO_CONFIG_DIR}/catalog"🤖 Prompt for AI Agents |
||||||
|
|
||||||
| # Copy over files | ||||||
| mv ${PRESTO_CONFIG_DIR}/clp.properties ${PRESTO_CONFIG_DIR}/catalog | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Fix shell quoting issue. Variables should be quoted to prevent potential issues. -mv ${PRESTO_CONFIG_DIR}/clp.properties ${PRESTO_CONFIG_DIR}/catalog
+mv "${PRESTO_CONFIG_DIR}/clp.properties" "${PRESTO_CONFIG_DIR}/catalog"📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,18 @@ | ||||||||||||||||||
| #!/bin/sh | ||||||||||||||||||
|
|
||||||||||||||||||
| # Exit on error | ||||||||||||||||||
| set -e | ||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||||||||||||||||
|
|
||||||||||||||||||
| PRESTO_CONFIG_DIR="/opt/presto-server/etc" | ||||||||||||||||||
|
|
||||||||||||||||||
| # Substitute environemnt variables in config template | ||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||||||||||||||||
| find /configs -type f | while read -r f; do | ||||||||||||||||||
| ( echo "cat <<EOF"; cat $f; echo "EOF" ) | sh > "${PRESTO_CONFIG_DIR}/$(basename "$f")" | ||||||||||||||||||
| done | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security risk: Avoid shell evaluation for template processing. The current approach using shell evaluation ( Consider using -# Substitute environemnt variables in config template
-find /configs -type f | while read -r f; do
- ( echo "cat <<EOF"; cat $f; echo "EOF" ) | sh > "${PRESTO_CONFIG_DIR}/$(basename "$f")"
-done
+# Substitute environment variables in config template
+find /configs -type f | while read -r f; do
+ envsubst < "$f" > "${PRESTO_CONFIG_DIR}/$(basename "$f")"
+done📝 Committable suggestion
Suggested change
🧰 Tools🪛 Shellcheck (0.10.0)[info] 10-10: Double quote to prevent globbing and word splitting. (SC2086) 🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| # Setup the config directory hierarchy | ||||||||||||||||||
| rm -f ${PRESTO_CONFIG_DIR}/catalog/* | ||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||||||||||||||||
|
|
||||||||||||||||||
| # Copy over files | ||||||||||||||||||
| mv ${PRESTO_CONFIG_DIR}/clp.properties ${PRESTO_CONFIG_DIR}/catalog | ||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| package: | ||
| storage_engine: "clp-s" | ||
| database: | ||
| type: "mariadb" | ||
| host: "${REPLACE_IP}" | ||
| port: 6001 | ||
| name: "clp-db" | ||
| query_scheduler: | ||
| host: "${REPLACE_IP}" | ||
| port: 6002 | ||
| jobs_poll_delay: 0.1 | ||
| num_archives_to_search_per_sub_job: 16 | ||
| logging_level: "INFO" | ||
| queue: | ||
| host: "${REPLACE_IP}" | ||
| port: 6003 | ||
| redis: | ||
| host: "${REPLACE_IP}" | ||
| port: 6004 | ||
| query_backend_database: 0 | ||
| compression_backend_database: 1 | ||
| reducer: | ||
| host: "${REPLACE_IP}" | ||
| base_port: 6100 | ||
| logging_level: "INFO" | ||
| upsert_interval: 100 | ||
| results_cache: | ||
| host: "${REPLACE_IP}" | ||
| port: 6005 | ||
| db_name: "clp-query-results" | ||
| stream_collection_name: "stream-files" | ||
| webui: | ||
| host: "localhost" | ||
| port: 6000 | ||
| logging_level: "INFO" | ||
| log_viewer_webui: | ||
| host: "localhost" | ||
| port: 6006 | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| SCRIPT_PATH="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
|
|
||
| function generate_sample_clp_config { | ||
| local ip=$(hostname -i) | ||
| local file="${SCRIPT_PATH}/clp-config.yml" | ||
| cp "$file" "${file}.bak" | ||
| sed -i "s|\${REPLACE_IP}|$ip|g" "$file" | ||
| echo "Replaced \${REPLACE_IP} with $ip in $file" | ||
| } | ||
|
|
||
| function update_metadata_config { | ||
| if [[ $# -ne 1 ]]; then | ||
| echo "Usage: update_metadata_config </path/to/clp-package>" | ||
| return 1 | ||
| fi | ||
|
|
||
| local clp_pkg_home=$1 | ||
| local clp_config_path="$(readlink -f ${clp_pkg_home})/etc/clp-config.yml" | ||
| local credential_path="$(readlink -f ${clp_pkg_home})/etc/credentials.yml" | ||
| host=$(python3 -c 'import sys, yaml; print(yaml.load(sys.stdin)["database"]["host"])' < "$clp_config_path") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Quote variables forwarded to -local clp_pkg_home=$1
-local clp_config_path="$(readlink -f ${clp_pkg_home})/etc/clp-config.yml"
+local clp_pkg_home="$1"
+local clp_config_path="$(readlink -f "$clp_pkg_home")/etc/clp-config.yml"Same adjustment applies to 🧰 Tools🪛 Shellcheck (0.10.0)[warning] 20-20: Declare and assign separately to avoid masking return values. (SC2155) [info] 20-20: Double quote to prevent globbing and word splitting. (SC2086) [warning] 21-21: Declare and assign separately to avoid masking return values. (SC2155) [info] 21-21: Double quote to prevent globbing and word splitting. (SC2086) 🤖 Prompt for AI Agents |
||
| port=$(python3 -c 'import sys, yaml; print(yaml.load(sys.stdin)["database"]["port"])' < "$clp_config_path") | ||
| name=$(python3 -c 'import sys, yaml; print(yaml.load(sys.stdin)["database"]["name"])' < "$clp_config_path") | ||
| user=$(python3 -c 'import sys, yaml; print(yaml.load(sys.stdin)["database"]["user"])' < "$credential_path") | ||
| password=$(python3 -c 'import sys, yaml; print(yaml.load(sys.stdin)["database"]["password"])' < "$credential_path") | ||
| echo "Metadata database host: $host" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace unsafe
-host=$(python3 -c 'import sys, yaml; print(yaml.load(sys.stdin)["database"]["host"])' < "$clp_config_path")
+host=$(python3 -c 'import sys, yaml; import yaml, sys; print(yaml.safe_load(sys.stdin)["database"]["host"])' < "$clp_config_path")Repeat for the other four calls ( 🤖 Prompt for AI Agents |
||
| echo "Metadata database port: $port" | ||
| echo "Metadata database name: $name" | ||
| echo "Metadata database user: $user" | ||
| echo "Metadata database password: $password" | ||
|
|
||
| local env_path="${SCRIPT_PATH}/../.env" | ||
| cp "$env_path" "${env_path}.bak" | ||
| sed -i "s|^PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_URL=.*|PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_URL=\"jdbc:mysql://${host}:${port}\"|" "$env_path" | ||
| sed -i "s/^PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_NAME=.*/PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_NAME=\"${name}\"/" "$env_path" | ||
| sed -i "s/^PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_USER=.*/PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_USER=\"${user}\"/" "$env_path" | ||
| sed -i "s/^PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_PASSWORD=.*/PRESTO_COORDINATOR_CONFIG_CLPPROPERTIES_METADATA_DATABASE_PASSWORD=\"${password}\"/" "$env_path" | ||
| sed -i "s|^CLP_PACKAGE_ARCHIVES=.*|CLP_PACKAGE_ARCHIVES=\"$(readlink -f ${clp_pkg_home})/var/data/archives/default\"|" "$env_path" | ||
| } | ||
|
|
||
| if declare -f "$1" > /dev/null; then | ||
| "$@" | ||
| else | ||
| echo "Error: '$1' is not a valid function name." | ||
| echo "Available functions:" | ||
| declare -F | awk '{print $3}' | ||
| exit 1 | ||
| fi | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| version: "3.9" | ||
|
|
||
| services: | ||
| presto-coordinator: | ||
| image: ghcr.io/y-scope/presto/coordinator:dev | ||
| entrypoint: ["/bin/bash", "-c", "/scripts/generate-configs.sh && /opt/entrypoint.sh"] | ||
| env_file: | ||
| - .env | ||
| volumes: | ||
| - ./coordinator/scripts:/scripts:ro | ||
| - coordinator-config:/opt/presto-server/etc | ||
| - ./coordinator/config-template:/configs:ro | ||
| networks: | ||
| - presto | ||
|
|
||
| presto-worker: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should define a @coderabbitai file an issue
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 42 @junhaoliao, I've filed issue #1504 to track adding a healthcheck to the Presto worker service. The issue has been assigned to you. |
||
| image: ghcr.io/y-scope/presto/prestissimo-worker:dev | ||
| depends_on: | ||
| presto-coordinator: | ||
| condition: service_started | ||
| entrypoint: ["/bin/bash", "-c", "/scripts/generate-configs.sh && /opt/entrypoint.sh"] | ||
| env_file: | ||
| - .env | ||
| volumes: | ||
| - ./worker/scripts:/scripts:ro | ||
| - worker-config:/opt/presto-server/etc | ||
| - ./worker/config-template:/configs:ro | ||
| - "${CLP_PACKAGE_ARCHIVES}:${CLP_PACKAGE_ARCHIVES}" | ||
| networks: | ||
| - presto | ||
|
|
||
| volumes: | ||
| coordinator-config: | ||
| worker-config: | ||
|
|
||
| networks: | ||
| presto: | ||
| driver: bridge | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| connector.name=clp | ||
|
|
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should set @coderabbitai file an issue
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 42 @junhaoliao, I've created issue #1503 to track the |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,7 @@ | ||||||
| discovery.uri=http://${PRESTO_COORDINATOR_SERVICENAME}:${PRESTO_COORDINATOR_HTTPPORT} | ||||||
| presto.version=REPLACE_ME | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Prefer an env placeholder over hard-coded Replacing -presto.version=REPLACE_ME
+presto.version=${PRESTO_VERSION}The script can then simply export 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| http-server.http.port=${PRESTO_WORKER_HTTPPORT} | ||||||
| shutdown-onset-sec=1 | ||||||
| register-test-functions=false | ||||||
| runtime-metrics-collection-enabled=false | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,5 @@ | ||||||||||||||||||
| node.environment=${PRESTO_COORDINATOR_CONFIG_NODEPROPERTIES_ENVIRONMENT} | ||||||||||||||||||
| node.internal-address=REPLACE_ME | ||||||||||||||||||
| node.location=${PRESTO_WORKER_CONFIG_NODEPROPERTIES_LOCATION} | ||||||||||||||||||
| node.id=REPLACE_ME | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Environment variable name may be misleading for the worker template.
-node.environment=${PRESTO_COORDINATOR_CONFIG_NODEPROPERTIES_ENVIRONMENT}
+node.environment=${PRESTO_WORKER_CONFIG_NODEPROPERTIES_ENVIRONMENT}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| mutable-config=true | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoted values in
.envare passed verbatim – will include the quotesDocker Compose does not strip
"in.env, soPRESTO_COORDINATOR_HTTPPORTwill become"8080"(quotes included).That breaks integer expectations in config.properties and JDBC URLs.
Strip the quotes or switch to YAML-style
environment:blocks.(Apply similarly to the remaining keys.)
Also applies to: 6-33
🧰 Tools
🪛 dotenv-linter (3.3.0)
[warning] 2-2: [QuoteCharacter] The value has quote characters (', ")
[warning] 3-3: [QuoteCharacter] The value has quote characters (', ")
🤖 Prompt for AI Agents