Skip to content
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

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

aczajkowski
Copy link
Member

@aczajkowski aczajkowski commented Mar 1, 2022

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:

# Hive/Iceberg
* Support S3 Http Proxy configuration for Trino

Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

LGTM

@losipiuk losipiuk requested a review from electrum March 1, 2022 17:56
@aczajkowski aczajkowski force-pushed the acz/s3_proxy_config branch from 887ab5f to be54cdc Compare March 1, 2022 18:57
@aczajkowski aczajkowski force-pushed the acz/s3_proxy_config branch 2 times, most recently from dafc98b to 692d228 Compare March 2, 2022 12:01
Copy link
Member

@losipiuk losipiuk left a 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?

@aczajkowski
Copy link
Member Author

aczajkowski commented Mar 2, 2022

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
No did not have enough time yet to test it manually.
I was thinking on doing such Integration test with MinIO and HTTP Proxy.
But i have doubts:

  • its going to be slow and memory consuming (and we are already on the edge of performance of our CI)
  • seems we will be mostly testing AWS client library code.

On our site we test if configuration is properly handled. But not how TrinoS3FileSystem is configured.
Possibly i could even should add a test for that. This would cover our code well and check if config settings are properly set to S3 Client through all layers.
WDYT ??

@losipiuk
Copy link
Member

losipiuk commented Mar 2, 2022

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 No did not have enough time yet to test it manually. I was thinking on doing such Integration test with MinIO and HTTP Proxy. But i have doubts:

  • its going to be slow and memory consuming (and we are already on the edge of performance of our CI)
  • seems we will be mostly testing AWS client library code.

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.

On our site we test if configuration is properly handled. But not how TrinoS3FileSystem is configured. Possibly i could even should add a test for that. This would cover our code well and check if config settings are properly set to S3 Client through all layers. WDYT ??

You mean tests like we have in TestTrinoS3FileSystem. Would be nice to have.

@aczajkowski aczajkowski force-pushed the acz/s3_proxy_config branch from 692d228 to 0f87ddd Compare March 2, 2022 19:01
@aczajkowski
Copy link
Member Author

aczajkowski commented Mar 2, 2022

@losipiuk

@losipiuk No did not have enough time yet to test it manually. I was thinking on doing such Integration test with MinIO and HTTP Proxy. But i have doubts:

  • its going to be slow and memory consuming (and we are already on the edge of performance of our CI)
  • seems we will be mostly testing AWS client library code.

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.

From my observations they are slower. But yes it's not going to be supper heavy.
Maybe let me rephrase my doubt. I think it's going to mostly test S3 Client from AWS.
Which i believe is well tested by AWS.

On our site we test if configuration is properly handled. But not how TrinoS3FileSystem is configured. Possibly i could even should add a test for that. This would cover our code well and check if config settings are properly set to S3 Client through all layers. WDYT ??

You mean tests like we have in TestTrinoS3FileSystem. Would be nice to have.

Yes just added it :)

@aczajkowski
Copy link
Member Author

aczajkowski commented Mar 3, 2022

@losipiuk I've adde Integration test using Apache HTTPD + MinIO to test S3 Proxy configuration.
It's on top of previous commit so previous review effects are untouched.

@aczajkowski aczajkowski force-pushed the acz/s3_proxy_config branch 2 times, most recently from 37b50d0 to 16f902e Compare March 3, 2022 14:15
@aczajkowski aczajkowski force-pushed the acz/s3_proxy_config branch from 16f902e to 0f87ddd Compare March 3, 2022 16:18
@aczajkowski
Copy link
Member Author

@losipiuk reverted to single commit (all changes connected to Proxy Integration Test dropped)

@losipiuk
Copy link
Member

losipiuk commented Mar 3, 2022

CI: #4936

@losipiuk losipiuk merged commit 132452e into trinodb:master Mar 3, 2022
@github-actions github-actions bot added this to the 373 milestone Mar 3, 2022
@findepi
Copy link
Member

findepi commented Mar 4, 2022

test coming here: #11310

@aczajkowski aczajkowski deleted the acz/s3_proxy_config branch March 21, 2022 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants