create parent dir also in parallel downloads, fixes #853 #854
Conversation
| truncateDestinationFileIfNecessary(); | ||
| } else { | ||
| if (!parentDirectory.mkdirs()) { | ||
| throw new AmazonClientException("Unable to create directory in the path" + parentDirectory.getAbsolutePath()); |
There was a problem hiding this comment.
Space or ": " needed in exception message
There was a problem hiding this comment.
Heh, I copied this from ServiceUtils. I'll fix the other place too.
| if (parentDirectory == null || parentDirectory.exists()) { | ||
| truncateDestinationFileIfNecessary(); | ||
| } else { | ||
| if (!parentDirectory.mkdirs()) { |
| @@ -217,7 +224,7 @@ private void combineFiles() throws Exception { | |||
| * operation), adjust the file length so that the part starts writing from | |||
| * the correct position. | |||
| */ | |||
| private void truncateDestinationFileIfNecessary() throws IOException { | |||
| private void truncateDestinationFileIfNecessary() { | |||
There was a problem hiding this comment.
Why remove the IOException and force up into the general exception?
There was a problem hiding this comment.
IOException was not thrown from this method. It only throws unchecked exceptions.
|
Minor optimization but it'd be nice to have the directory creation for both in a single util method so we're consistent with it's handling. Perhaps a |
|
Actually looking at this again, I don't think we need this. In the most recent version of the SDK we made an update to the multipart download so that file parts are downloaded in the same directory as the target file (ie: dstFile) in this case the parent should already exist. If you upgrade to 1.11.37 you should not need this explicit create for multi-part files. |
|
Hmm, I'd swear I've tested it with the most recent version too and it failed. I'll have a look at it again after the weekend. |
|
1.11.37 was only released yesterday. |
|
No, 37 is still broken. What you say about the new behaviour is all true, except that the exception is thrown from the locking code, so potentially before the parent dir gets asynchonously created in the download callables. |
|
That makes sense, I guess we could have solved it by just pushing the call to Looks good to me - this fix should go out with our next release. |
| @@ -197,7 +196,10 @@ private void downloadInParallel(int partCount) throws Exception { | |||
| * Merges all the individual part Files into dstFile | |||
| */ | |||
| private void combineFiles() throws Exception { | |||
| truncateDestinationFileIfNecessary(); | |||
| if (!ServiceUtils.createParentDirectoryIfNecessary(dstfile)) { | |||
There was a problem hiding this comment.
This won't fix the problem. futureFiles starts download concurrently, which may happen before this is excuted
There was a problem hiding this comment.
The future file get also creates the parent directory if necessary I believe. So which ever one starts first should create the parent directory and hopefully resolve the issue.
There was a problem hiding this comment.
Unfortunately, the temp file is created before the ServiceUtils parent directory creation is done. So, yeah, the fix is not complete.
No description provided.