Fix multipart upload pause #1536
Conversation
raphkim
added
s3
raphkim
requested review from
jpignata,
jamesonwilliams,
desokroshan,
TrekSoft and
a team
|
|
||
| public final class PauseTransferIntegrationTest extends S3IntegrationTestBase { | ||
|
|
||
| private static final String bucketName = "amazon-transfer-util-integ-test-" + new Date().getTime(); |
There was a problem hiding this comment.
Can we name it like BUCKET_NAME to be consistent with convention and rest of the codebase?
There was a problem hiding this comment.
yea oops. A mistake that was copy + pasted from my previous mistake. will fix them
| @@ -144,8 +144,6 @@ public Boolean call() { | |||
| */ | |||
| updater.updateState(download.id, TransferState.WAITING_FOR_NETWORK); | |||
| LOGGER.debug("Network Connection Interrupted: " + "Moving the TransferState to WAITING_FOR_NETWORK"); | |||
| ProgressEvent resetEvent = new ProgressEvent(0); | |||
| resetEvent.setEventCode(ProgressEvent.RESET_EVENT_CODE); | |||
There was a problem hiding this comment.
Shouldn't we be resetting the progress upon loss of network and then pass reserEvent to the progressListener?
| @@ -310,8 +315,6 @@ private Boolean uploadSinglePartAndWaitForCompletion() { | |||
| */ | |||
| updater.updateState(upload.id, TransferState.WAITING_FOR_NETWORK); | |||
| LOGGER.debug("Network Connection Interrupted: " + "Moving the TransferState to WAITING_FOR_NETWORK"); | |||
| ProgressEvent resetEvent = new ProgressEvent(0); | |||
| resetEvent.setEventCode(ProgressEvent.RESET_EVENT_CODE); | |||
There was a problem hiding this comment.
Same as above. If eventCode is not being used anywhere should we consider removing it altogether from the ProgressEvent class?
| if (TransferState.PAUSED.equals(download.state)) { | ||
| LOGGER.info("Transfer is " + download.state); | ||
| ProgressEvent resetEvent = new ProgressEvent(0); | ||
| resetEvent.setEventCode(ProgressEvent.RESET_EVENT_CODE); |
There was a problem hiding this comment.
Are we not resetting on pause on anymore? If so, do we need to update documentation?
There was a problem hiding this comment.
if you look closely, the lines i removed were noops. Im not sure if the original intent was to pass in a new ProgressEvent(0) with reset flag, but this resetEvent object never gets used and a new progress event without flag is reinstantiated instead. Perhaps a different bug, but I didn't want to introduce yet another behavior change in this PR.
|
This PR contained removal of some lines that SEEMED to be doing something important but was actually not doing anything: The first two lines don't do anything, since This is probably an important issue of its own that requires its own investigation, so I decided to leave the redundant lines for now to revisit in the future with a separate PR. |
* Fix multipart upload interruption (pause/cancel) * Change constant name according to convention * Re-add reset code lines to avoid confusion
Issue #, if available:
#1500
Description of changes:
TransferUtility#pause(int)was not behaving correctly when uploading a multipart (> 5MB) item. This was due to interruption detection (due to pause) code returning early before actually canceling the individualUploadPartTasktasks in the upload. The same result would have been seen forTransferUtility#cancel(int)for a large upload.Additionally to fixing the above bug, this PR also contains fix to mitigate the effects of race condition observed when pausing multipart upload and resuming it instantly. Previously, the transfer state would be set to
PAUSEDorCANCELEDas soon as the method was called even before all of the task was interrupted. NowPENDING_PAUSEandPENDING_CANCEL(already present in the codebase but unused) states are used to make sure that the task is successfully interrupted before the state is updated to its final state.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.