-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add S3 Http Proxy configuration support #11255
Conversation
7faeb10
to
887ab5f
Compare
plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3/HiveS3Config.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/TestHiveS3Config.java
Outdated
Show resolved
Hide resolved
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.
LGTM
887ab5f
to
be54cdc
Compare
plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3/HiveS3Config.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3/HiveS3Config.java
Outdated
Show resolved
Hide resolved
dafc98b
to
692d228
Compare
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.
LGTM. Is there a way to test it? Maybe an integration test with MinIO + a HTTP proxy container (not sure if worth it).
Also did you have a chance to at least test it manually?
@losipiuk
On our site we test if configuration is properly handled. But not how TrinoS3FileSystem is configured. |
Why do you think it would be slow. It would as fast as any other integration test where we put up MinIO container. It may be a bit painful to code - but runtime-wise I do not expect it will be a problem.
You mean tests like we have in |
692d228
to
0f87ddd
Compare
From my observations they are slower. But yes it's not going to be supper heavy.
Yes just added it :) |
@losipiuk I've adde Integration test using Apache HTTPD + MinIO to test S3 Proxy configuration. |
37b50d0
to
16f902e
Compare
16f902e
to
0f87ddd
Compare
@losipiuk reverted to single commit (all changes connected to Proxy Integration Test dropped) |
CI: #4936 |
test coming here: #11310 |
Description
Add S3 Http Proxy configuration support for Trino
Documentation
(X) No documentation is needed.
(V) Sufficient documentation is included in this PR.
(X) Documentation PR is available with #prnumber.
(X) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
(X) No release notes entries required.
(V) Release notes entries required with the following suggested text: