-
Notifications
You must be signed in to change notification settings - Fork 804
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
[TM DownloadFile Pause and Resume] Part 1: Add configuration to enable overwriting existing files #3125
[TM DownloadFile Pause and Resume] Part 1: Add configuration to enable overwriting existing files #3125
Conversation
core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java
Outdated
Show resolved
Hide resolved
core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java
Outdated
Show resolved
Hide resolved
core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java
Show resolved
Hide resolved
...e/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformer.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/software/amazon/awssdk/core/internal/async/FileAsyncResponseTransformer.java
Outdated
Show resolved
Hide resolved
core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java
Outdated
Show resolved
Hide resolved
core/sdk-core/src/main/java/software/amazon/awssdk/core/FileTransformerConfiguration.java
Outdated
Show resolved
Hide resolved
private long determineFilePositionToWrite(Path path) { | ||
if (fileModificationOption == CREATE_OR_APPEND_EXISTING) { | ||
try { | ||
return Files.size(path); | ||
} catch (NoSuchFileException e) { | ||
// Ignore | ||
} catch (IOException exception) { | ||
throw SdkClientException.create("Cannot determine the current file size " + path, exception); | ||
} |
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.
This seems like an expensive thing to do all the time (e.g. if the file is always being created for the first time). Is there any way we can defer this until we need it or use a solution that doesn't rely on catching NoSuchFileException
?
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.
File position has to be determined in the ctor because otherwise retry would not work for appending in the case where certain bytes have been written. We could callFiles.notExists
, but it also relies on catching NoSuchFileException behind the scene.
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.
Hmm couldn't we do it at the time we open the file?
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.
No not really, we open the file in onStream
method, so the position we get at this point may not be correct if the previous attempt had written a few bytes already before it failed.
6c8a211
to
1576120
Compare
Changed the target branch to
|
Kudos, SonarCloud Quality Gate passed! |
*/ | ||
public static FileTransformerConfiguration defaultCreateOrAppend() { | ||
return builder().fileWriteOption(FileWriteOption.CREATE_OR_APPEND_EXISTING) | ||
.failureBehavior(FailureBehavior.LEAVE) |
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.
Any specific reason for this default?
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.
Yeah, retry would not work as expected if we delete the file when there is an exception.
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.
But it also might not work as expected if we started appending, failed and then on a retry reappended the same data.
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.
The initial position is recorded in the ctor, so we will always append on the same offset on retries (overwrite whatever bytes that have been written in the previous attempt)
if (configuration.fileWriteOption() == CREATE_OR_APPEND_EXISTING) { | ||
try { | ||
return Files.size(path); | ||
} catch (NoSuchFileException e) { | ||
// Ignore | ||
} catch (IOException exception) { | ||
throw SdkClientException.create("Cannot determine the current file size " + path, exception); | ||
} | ||
} | ||
return 0L; |
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.
Can we add a backlog item to revisit this? It makes me nervous to rely on catching an exception on a possibly hot code path.
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.
We could call Files#exists
first but it uses the same try-catch though... I'm not sure how we can optimize it, but yeah, I can add a TODO
* [TM DownloadFile Pause and Resume] Part 1: Add configuration to enable overwriting existing files (#3125) * Expose an option to overwrite an existing file in FileAsyncResponseTransformer * Add changelog entries and make TM use CREATE_OR_REPLACE_EXISTING write option by default * Address feedback * Update and address feedback * [TM DownloadFile Pause and Resume] Part 2: Implement pause for downloadFile operation (#3094) * Part 1: Implement pause for downloadFile operation * Address feedback * Refactor the logic * Address feedback * Fix merging issue * [TM DownloadFile Pause and Resume] Part 3: Implement resumeDownloadFile (#3157) * Implement resumeDownloadFile * Move test code around * Address feedback * Fix flaky test * fix flaky integ test * add changelog entry * Troubleshooting flaky test * Add file length check when checking if file has modified or not * Address feedback
Motivation and Context
#3108
#2300
Modifications
Expose an option to overwrite an existing file in
FileAsyncResponseTransformer
.Note that it's not added to
FileResponseTransformer
right now because it's not trivial to do so. I will create a backlog item for it.Testing
Added unit tests and integ tests
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsscripts/new-change
script and following the instructions. Commit the new file created by the script in.changes/next-release
with your changes.License