Skip to content

Conversation

@singhpk234
Copy link
Contributor

@singhpk234 singhpk234 commented Jul 27, 2022

About the Change :

Presently whenever a failure is observed in any of the completableFutures in uploadParts(), it causes further completable futures fail with FileNotFoundException, when it tries to create a request for it using RequestBody.fromFile(file), as all the stagings files deleted as part of abortUpload which is called on failure of first completableFuture.

UploadPartResponse response = s3.uploadPart(uploadRequest, RequestBody.fromFile(f));

I think this is what was also highlighted in @stevenzwu 's observation here (#4168 (comment))

This change attempts :

  • To avoid unnecessary deletes being called for all staging files on every completableFutures which fail due to fileNotFoundException.
  • Cancels the remaining futures in which would not be required as the abort is called, reduces number of calls to abort multi-part upload as well.

Testing Done

Existing UT's + a UT to see abort is called only once rather than atleast once.

cc @rdblue @jackye1995 @RussellSpitzer @stevenzwu @amogh-jahagirdar @rajarshisarkar

@github-actions github-actions bot added the AWS label Jul 27, 2022
@singhpk234
Copy link
Contributor Author

This PR can also fix the flakyness of TestS3OutputStream#testAbortAfterFailedPartUpload in recent runs of master.

@RussellSpitzer RussellSpitzer merged commit aae6155 into apache:master Aug 1, 2022
@RussellSpitzer
Copy link
Member

Thanks @singhpk234 and thanks @saswata-dutta for review!

abmo-x pushed a commit to abmo-x/iceberg that referenced this pull request Aug 2, 2022
@singhpk234
Copy link
Contributor Author

singhpk234 commented Aug 2, 2022

Thanks @saswata-dutta , @RussellSpitzer for the review :) !

Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing this

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants