-
Notifications
You must be signed in to change notification settings - Fork 91
feat(presto-clp): Update docs and config generation to support reading archives from S3 and Presto split-filtering config. #1228
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 14 commits
af8ce88
e0a1820
4deb2ba
728efe2
2ba53ca
aa3e18d
67e16ce
ba16edf
78139be
8d6744d
2afac89
d4bf46a
5e13836
d7478f8
14b3a80
c09641b
d59af37
80959fc
ada03bb
9d92fc9
968c29c
cdaf3f5
40c352c
e5e1ed6
b9a898f
e012fb0
354a1aa
d1aa25b
29c4232
c4bcb8b
5cb13b9
570c18a
9842ef6
f769282
a388247
72ae05b
ccaf9a3
34617b9
b3694b7
6bcacfb
0a670b0
33592c9
bc86d60
3402f9c
2ac911a
06b5771
bc90d7f
2b3c624
8ac0b03
2306ba1
f4d08b4
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 | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -48,13 +48,19 @@ update_config_file() { | |||||||||||||||||||||||||||||
| local file_path=$1 | ||||||||||||||||||||||||||||||
| local key=$2 | ||||||||||||||||||||||||||||||
| local value=$3 | ||||||||||||||||||||||||||||||
| local sensitive=${4:-false} | ||||||||||||||||||||||||||||||
|
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) Document the new "sensitive" parameter in update_config_file. The function signature gained a fourth parameter, but the header comment still lists only three. Add the param to avoid misuse by future callers. # @param $1 Path to the properties file.
# @param $2 The key to set.
# @param $3 The value to set.
+# @param $4 (optional) "true" to mask the value when logging📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if grep --quiet "^${key}=.*$" "$file_path"; then | ||||||||||||||||||||||||||||||
| sed --in-place "s|^${key}=.*|${key}=${value}|" "$file_path" | ||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||
| echo "${key}=${value}" >>"$file_path" | ||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||
|
Comment on lines
53
to
56
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 Make key/value matching and replacement regex-safe; avoid failures on missing files. Keys like node.internal-address contain dots that are regex metacharacters. Using grep/sed with unescaped keys can produce false positives. Also, grep on a non-existent file will fail under set -e. Guard the file and use fixed-string grep; escape replacement metacharacters in the value. - if grep --quiet "^${key}=.*$" "$file_path"; then
- sed --in-place "s|^${key}=.*|${key}=${value}|" "$file_path"
- else
- echo "${key}=${value}" >>"$file_path"
- fi
+ # Ensure the file exists to avoid grep/sed failures under `set -e`
+ [[ -f "$file_path" ]] || : >"$file_path"
+ # Match keys literally to avoid regex surprises with dots, etc.
+ if grep -F --quiet "^${key}=" "$file_path"; then
+ # Escape replacement metacharacters in value for sed
+ local escaped_value=${value//\\/\\\\}
+ escaped_value=${escaped_value//|/\\|}
+ escaped_value=${escaped_value//&/\\&}
+ local escaped_key
+ escaped_key=$(printf '%s' "$key" | sed -e 's/[.[\*^$+?{}|()]/\\&/g')
+ sed --in-place "s|^${escaped_key}=.*|${key}=${escaped_value}|" "$file_path"
+ else
+ echo "${key}=${value}" >>"$file_path"
+ fi
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
| log "INFO" "Set ${key}=${value} in ${file_path}" | ||||||||||||||||||||||||||||||
| if [[ "$sensitive" == "true" ]]; then | ||||||||||||||||||||||||||||||
| local masked="****${value: -4}" | ||||||||||||||||||||||||||||||
| log "INFO" "Set ${key}=${masked} in ${file_path}" | ||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||
| log "INFO" "Set ${key}=${value} in ${file_path}" | ||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||
|
anlowee marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| apt-get update && apt-get install --assume-yes --no-install-recommends jq wget | ||||||||||||||||||||||||||||||
|
|
@@ -74,6 +80,26 @@ done | |||||||||||||||||||||||||||||
| rm -f "${PRESTO_CONFIG_DIR}/catalog/"* | ||||||||||||||||||||||||||||||
| mv "${PRESTO_CONFIG_DIR}/clp.properties" "${PRESTO_CONFIG_DIR}/catalog" | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Update clp.properties | ||||||||||||||||||||||||||||||
| readonly CLP_PROPERTIES_FILE="/opt/presto-server/etc/catalog/clp.properties" | ||||||||||||||||||||||||||||||
| if [ "${PRESTO_WORKER_CLPPROPERTIES_STORAGE_TYPE:-}" = "s3" ]; then | ||||||||||||||||||||||||||||||
| log "INFO" "Enable S3 support" | ||||||||||||||||||||||||||||||
| for var in PRESTO_WORKER_CLPPROPERTIES_S3_AUTH_PROVIDER \ | ||||||||||||||||||||||||||||||
| PRESTO_WORKER_CLPPROPERTIES_S3_ACCESS_KEY_ID \ | ||||||||||||||||||||||||||||||
| PRESTO_WORKER_CLPPROPERTIES_S3_SECRET_ACCESS_KEY \ | ||||||||||||||||||||||||||||||
| PRESTO_WORKER_CLPPROPERTIES_S3_END_POINT; do | ||||||||||||||||||||||||||||||
| if [ -z "${!var:-}" ]; then | ||||||||||||||||||||||||||||||
| log "ERROR" "Missing required env var: $var" | ||||||||||||||||||||||||||||||
| exit 1 | ||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||
| done | ||||||||||||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||
| update_config_file "$CLP_PROPERTIES_FILE" "clp.storage-type" "${PRESTO_WORKER_CLPPROPERTIES_STORAGE_TYPE}" | ||||||||||||||||||||||||||||||
| update_config_file "$CLP_PROPERTIES_FILE" "clp.s3-auth-provider" "${PRESTO_WORKER_CLPPROPERTIES_S3_AUTH_PROVIDER}" | ||||||||||||||||||||||||||||||
| update_config_file "$CLP_PROPERTIES_FILE" "clp.s3-access-key-id" "${PRESTO_WORKER_CLPPROPERTIES_S3_ACCESS_KEY_ID}" true | ||||||||||||||||||||||||||||||
| update_config_file "$CLP_PROPERTIES_FILE" "clp.s3-end-point" "${PRESTO_WORKER_CLPPROPERTIES_S3_END_POINT}" | ||||||||||||||||||||||||||||||
| update_config_file "$CLP_PROPERTIES_FILE" "clp.s3-secret-access-key" "${PRESTO_WORKER_CLPPROPERTIES_S3_SECRET_ACCESS_KEY}" true | ||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Update config.properties | ||||||||||||||||||||||||||||||
| readonly CONFIG_PROPERTIES_FILE="/opt/presto-server/etc/config.properties" | ||||||||||||||||||||||||||||||
| version=$(get_coordinator_version "$CONFIG_PROPERTIES_FILE") | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.