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

Extend test coverage for S3 streaming upload #10797

Closed
losipiuk opened this issue Jan 25, 2022 · 2 comments · Fixed by #11607
Closed

Extend test coverage for S3 streaming upload #10797

losipiuk opened this issue Jan 25, 2022 · 2 comments · Fixed by #11607
Assignees
Labels

Comments

@losipiuk
Copy link
Member

Extra testing

#10710 showed that we are lacking test coverage for S3 streaming upload.
We should write testcases which upload files of multiple sizes with S3 streaming enabled (default currently). Specifically

For tests which talk to real S3 endpoint on AWS we should extend AbstractTestHiveFileSystemS3 (this test is run for master and for PRs originated from trinodb/trino).

AbstractTestHiveFileSystemS3 is a low-level test in which we directly create PageSink etc. To have more blackbox coverage we can also write an integration test which uses MiniIO based QueryRunner and consumes SQL statements to trigger writing.
For that BaseTestHiveOnDataLake can be extended.

Extra guards in TrinoS3FileSystem

Additionally it would be nice to add extra guards to TrinoS3FileSystem filesystem which ensure that close() would not call finishUpload if file upload is alredy considered completed; when code path which uploads small files without multipart mechanism
is executed. Having such guard would prevent #10710 from happening.

cc: @linzebing @findepi @joshthoward

@losipiuk
Copy link
Member Author

Reopened because added test proved to be flaky and was disabled.

@linzebing
Copy link
Member

Closed via #11678

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants