Skip to content

feat(presto-clp): Add support for custom S3 endpoint URLs and buckets. #1917

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.

Sign up for GitHub

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

Merged
20001020ycx merged 16 commits into from
Feb 20, 2026
Merged

feat(presto-clp): Add support for custom S3 endpoint URLs and buckets. #1917

Changes from 5 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2be8401
add code for update clp.properties in the velox worker
20001020ycx Jan 27, 2026
6298bc4
fix error handling
20001020ycx Jan 27, 2026
9319488
delete unused var
20001020ycx Jan 27, 2026
8511646
address comment
20001020ycx Jan 27, 2026
f8ee2e5
nit fix
20001020ycx Jan 27, 2026
fc402c9
Update tools/deployment/presto-clp/scripts/init.py
20001020ycx Jan 27, 2026
064feb8
Update tools/deployment/presto-clp/scripts/init.py
20001020ycx Jan 27, 2026
db8b8f7
address review comment
20001020ycx Jan 28, 2026
b1a0881
nit
20001020ycx Jan 28, 2026
469aa3b
Update tools/deployment/presto-clp/scripts/init.py
20001020ycx Jan 28, 2026
ae56023
Merge branch 'main' into 2026-01-27-update-velox-conf
junhaoliao Jan 28, 2026
8520ff9
Merge branch 'main' into 2026-01-27-update-velox-conf
junhaoliao Jan 28, 2026
90cc4b5
Merge branch 'main' into 2026-01-27-update-velox-conf
junhaoliao Jan 29, 2026
a080de5
Merge branch 'main' into 2026-01-27-update-velox-conf
junhaoliao Jan 29, 2026
ed7feb7
Merge branch 'main' into 2026-01-27-update-velox-conf
20001020ycx Feb 19, 2026
c859616
Merge branch 'main' into 2026-01-27-update-velox-conf
20001020ycx Feb 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions tools/deployment/presto-clp/scripts/init.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
PRESTO_QUERY_MEMORY_RATIO = 0.5
PRESTO_SYSTEM_MEMORY_RATIO = 0.9

# S3 URL constant
AWS_ENDPOINT = "amazonaws.com"
Comment thread
20001020ycx marked this conversation as resolved.
Resolved

# Set up console logging
logging_console_handler = logging.StreamHandler()
logging_formatter = logging.Formatter(
Expand Down Expand Up @@ -260,9 +263,6 @@ def _add_clp_s3_env_vars(
s3_bucket = _get_required_config_value(
clp_config, f"{s3_config_key}.bucket", clp_config_file_path
)
s3_region_code = _get_required_config_value(
clp_config, f"{s3_config_key}.region_code", clp_config_file_path
)

aws_auth_key = f"{s3_config_key}.aws_authentication"
aws_auth_type_key = f"{aws_auth_key}.type"
Expand All @@ -284,15 +284,38 @@ def _add_clp_s3_env_vars(
except KeyError:
return False

s3_endpoint_url = _get_config_value(clp_config, f"{s3_config_key}.endpoint_url")
s3_region_code = _get_config_value(clp_config, f"{s3_config_key}.region_code")
s3_end_point = _handle_s3_endpoint_url(s3_endpoint_url, s3_region_code, s3_bucket)

env_vars["PRESTO_WORKER_CLPPROPERTIES_S3_AUTH_PROVIDER"] = "clp_package"
env_vars["PRESTO_WORKER_CLPPROPERTIES_S3_ACCESS_KEY_ID"] = s3_access_key_id
s3_end_point = f"https://{s3_bucket}.s3.{s3_region_code}.amazonaws.com/"
env_vars["PRESTO_WORKER_CLPPROPERTIES_S3_BUCKET"] = s3_bucket
env_vars["PRESTO_WORKER_CLPPROPERTIES_S3_END_POINT"] = s3_end_point
env_vars["PRESTO_WORKER_CLPPROPERTIES_S3_SECRET_ACCESS_KEY"] = s3_secret_access_key

return True


def _handle_s3_endpoint_url(
Comment thread
20001020ycx marked this conversation as resolved.
Resolved
endpoint_url: str | None, region_code: str | None, bucket_name: str
) -> str:
"""
Returns the S3 endpoint URL, or constructs an AWS S3 URL if not provided.

:param endpoint_url:
Comment thread
20001020ycx marked this conversation as resolved.
Resolved
:param region_code: AWS region code, or None.
Comment thread
20001020ycx marked this conversation as resolved.
Resolved
:param bucket_name: S3 bucket name (used to construct AWS S3 URL).
Comment thread
20001020ycx marked this conversation as resolved.
Resolved
:return: The S3 endpoint URL.
"""
if endpoint_url is not None:
return endpoint_url
Comment thread
20001020ycx marked this conversation as resolved.
Resolved
Comment thread
20001020ycx marked this conversation as resolved.
Resolved

if region_code is None:

@junhaoliao junhaoliao Jan 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this is needed to support non-AWS S3 endpoints. However, omitting a region is non-standard for AWS S3. Historically, some AWS SDKs would fall back to us-east-1 when no region was specified, but this behavior should not be relied upon.

Should we add a log message when neither endpoint_url nor region_code is provided (here)?

@junhaoliao junhaoliao Jan 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g.
Neither 'endpoint_url' nor 'region_code' specified in S3 config. Using region-less S3 endpoint which may not work for all buckets.

@20001020ycx 20001020ycx Jan 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As suggested by @gibber9809 , this behavior is followed by generate_s3_url in s3_utils.py so that CLP package always make consistent assumption.

I am okay with adding the log message when neither 'endpoint_url' nor 'region_code' is specified in S3 config as a warnning. But should we keep the same behavior as the changes in this PR?

And thanks for the insight on this S3 support!

@junhaoliao junhaoliao Jan 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see what @gibber9809 decides on

@gibber9809 gibber9809 Jan 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for slow response. Yeah logging a warning in this case sounds reasonable. It probably also makes sense to log the same warning during the package startup whenever 'endpoint_url' and 'region_code' are missing from the package config for the same reason.

@gibber9809 gibber9809 Jan 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be fine tackling this in a separate PR though that adds the warning both here and in the package startup script.

@junhaoliao junhaoliao Jan 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai we will merge the current PR first and add the warning messages later. let's create an issue about adding a warning message both here and in the package startup script

@coderabbitai coderabbitai Bot Jan 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Create a GitHub issue for adding warning messages about missing S3 endpoint_url and region_code

gh issue create \
  --repo y-scope/clp \
  --title "Add warning when S3 config lacks both endpoint_url and region_code" \
  --body "## Context

When neither \`endpoint_url\` nor \`region_code\` is specified in the S3 configuration, the code constructs a region-less S3 endpoint (e.g., \`https://{bucket}.s3.amazonaws.com\`). This behavior is non-standard for AWS S3 and may not work for all buckets, as AWS typically requires a region to be specified.

This behavior was implemented to maintain consistency with [\`generate_s3_url\`](https://github.com/y-scope/clp/blob/01d35a672c2f6fb892912121800b7a85f7448b23/components/clp-py-utils/clp_py_utils/s3_utils.py#L257) in \`s3_utils.py\`.

## Task

Add warning log messages in the following two locations when neither \`endpoint_url\` nor \`region_code\` is provided:

1. **Presto init script**: \`tools/deployment/presto-clp/scripts/init.py\` in the \`_resolve_s3_endpoint_url\` function
2. **Package startup script**: Where S3 configuration is processed during CLP package startup

## Suggested warning message

\`\`\`
Neither 'endpoint_url' nor 'region_code' specified in S3 config. Using region-less S3 endpoint which may not work for all buckets.
\`\`\`

## References

- PR `#1917`: https://github.com/y-scope/clp/pull/1917
- Discussion: https://github.com/y-scope/clp/pull/1917#discussion_r2734254388
- Requested by: `@junhaoliao`" \
  --assignee junhaoliao

Length of output: 42

@junhaoliao I've created issue #1932 to track adding the warning messages for S3 configurations that lack both endpoint_url and region_code. The issue has been assigned to you and includes references back to this PR and discussion.

#1932

🧬 Code Graph Analysis Results

<no_relevant_code_snippets>

</no_relevant_code_snippets>

🧠 Learnings used 
Learnt from: hoophalab
Repo: y-scope/clp PR: 1767
File: components/clp-py-utils/clp_py_utils/clp_config.py:580-585
Timestamp: 2025-12-10T23:32:24.363Z
Learning: In PR `#1767`, custom S3 endpoint support was added to CLP. The S3Config.region_code field is now optional (NonEmptyStr | None) because custom S3-compatible endpoints (MinIO, LocalStack, etc.) use path-style URLs and don't require AWS region codes. Only AWS S3 endpoints require region_code. Presto integration still requires region_code because it only works with AWS S3.

Learnt from: haiqi96
Repo: y-scope/clp PR: 852
File: components/clp-package-utils/clp_package_utils/scripts/native/compress.py:151-160
Timestamp: 2025-04-25T20:46:20.140Z
Learning: For S3 URLs without region specifications (legacy global endpoints), either assign a default region (us-east-1) or throw a clear error message requiring region specification in the URL. This addresses validation issues in components like S3InputConfig that require a non-nullable region string.

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.

return f"https://{bucket_name}.s3.{AWS_ENDPOINT}/"
Comment thread
20001020ycx marked this conversation as resolved.
Resolved
return f"https://{bucket_name}.s3.{region_code}.{AWS_ENDPOINT}/"


def _add_memory_env_vars(env_vars: dict[str, str]) -> bool:
"""
Adds memory-related environment variables based on Presto guidelines.
Expand Down Expand Up @@ -359,6 +382,7 @@ def _generate_worker_clp_properties(
"clp.storage-type": "PRESTO_WORKER_CLPPROPERTIES_STORAGE_TYPE",
"clp.s3-auth-provider": "PRESTO_WORKER_CLPPROPERTIES_S3_AUTH_PROVIDER",
"clp.s3-access-key-id": "PRESTO_WORKER_CLPPROPERTIES_S3_ACCESS_KEY_ID",
"clp.s3-bucket": "PRESTO_WORKER_CLPPROPERTIES_S3_BUCKET",
"clp.s3-end-point": "PRESTO_WORKER_CLPPROPERTIES_S3_END_POINT",
"clp.s3-secret-access-key": "PRESTO_WORKER_CLPPROPERTIES_S3_SECRET_ACCESS_KEY",
}
Expand Down
Loading