feat: add headers option to MPU #2303
Conversation
|
This looks larger than it really is due to the fact that I removed |
| const fakeUtil = Object.assign({}, util); | ||
| fakeUtil.noop = util.noop; | ||
|
|
||
| class FakeServiceObject extends ServiceObject<FakeServiceObject, BaseMetadata> { | ||
| calledWith_: IArguments; | ||
| constructor(config: ServiceObjectConfig) { | ||
| super(config); | ||
| // eslint-disable-next-line prefer-rest-params | ||
| this.calledWith_ = arguments; | ||
| } | ||
| } | ||
|
|
||
| class FakeAcl { | ||
| calledWith_: Array<{}>; | ||
| constructor(...args: Array<{}>) { | ||
| this.calledWith_ = args; | ||
| } | ||
| } | ||
|
|
||
| class FakeFile { | ||
| calledWith_: IArguments; | ||
| bucket: Bucket; | ||
| name: string; | ||
| options: FileOptions; | ||
| metadata: FileMetadata; | ||
| createWriteStream: Function; | ||
| isSameFile = () => false; | ||
| constructor(bucket: Bucket, name: string, options?: FileOptions) { | ||
| // eslint-disable-next-line prefer-rest-params |
There was a problem hiding this comment.
My favorite part of the PR 💣
src/transfer-manager.ts
Outdated
| @@ -664,8 +696,12 @@ export class TransferManager { | |||
| let partNumber = 1; | |||
| let promises: Promise<void>[] = []; | |||
| try { | |||
| if (options.abortExisting && options.uploadId) { | |||
There was a problem hiding this comment.
How does this option work? If I set abortExisting to true, and there's an upload ID, it will cancel the the upload? Shouldn't it only do this if it fails?
There was a problem hiding this comment.
oh, looks like you can abort an upload using uploadFileInChunks when you set abortExisting=True and uploadId is set; based on
[abortExisting] boolean to indicate if an in progress upload should be aborted. uploadID must also be supplied in order to abort the upload.
I guess if this delete in context of this method; the upload id would be known and isn't required like this.
There was a problem hiding this comment.
@danielduhh correct the user would have to pass back the uploadId and set the abortExisting option. The way it works currently is if there is a failure the return has the uploadId and mapping of the completed parts that were uploaded.
test/transfer-manager.ts
Outdated
| ); | ||
| }); | ||
|
|
||
| it('should call abortUpload when passed the option and uploadID', async () => { |
There was a problem hiding this comment.
This seems like a hack? Why not just expose the abort API and ask for an id? We could document that users should catch failures and call this method (since we're not doing it for them)/
There was a problem hiding this comment.
When we implemented this we decided not to expose the actual XML implementation to avoid users accessing it directly. This was done because at the time we did not know if other MPU implementations would come along and the default would be changed (i.e. JSON).
There was a problem hiding this comment.
Yeah it feels odd to have this API do two completely different things based on the parameters.
Instead, I would consider following python and catching any errors thrown by MPU with a note:
The library will attempt to cancel uploads that fail due to an exception.
If the upload fails in a way that precludes cancellation, such as a
hardware failure, process termination, or power outage, then the incomplete
upload may persist indefinitely. To mitigate this, set the
`AbortIncompleteMultipartUpload` with a nonzero `Age` in bucket lifecycle
rules, or refer to the XML API documentation linked above to learn more
about how to list and delete individual downloads.
```
There was a problem hiding this comment.
This goes back to the debate of autodelete vs not. If we want to proceed with autodelete, I'll just move the logic internally to call the delete function.
There was a problem hiding this comment.
I set up a discussion with @andrewsg and @ddelgrosso1 to suss this out today; will report back;
| * | ||
| * @returns {Promise<void>} | ||
| */ | ||
| async abortUpload(): Promise<void> { |
There was a problem hiding this comment.
This entire XML implementation is not exported and therefore not accessible.
src/transfer-manager.ts
Outdated
| @@ -664,8 +696,12 @@ export class TransferManager { | |||
| let partNumber = 1; | |||
| let promises: Promise<void>[] = []; | |||
| try { | |||
| if (options.abortExisting && options.uploadId) { | |||
There was a problem hiding this comment.
oh, looks like you can abort an upload using uploadFileInChunks when you set abortExisting=True and uploadId is set; based on
[abortExisting] boolean to indicate if an in progress upload should be aborted. uploadID must also be supplied in order to abort the upload.
I guess if this delete in context of this method; the upload id would be known and isn't required like this.
| * @property {object} [headers] headers to be sent when initiating the multipart upload. | ||
| * See {@link https://cloud.google.com/storage/docs/xml-api/post-object-multipart#request_headers| Request Headers: Initiate a Multipart Upload} | ||
| * @property {boolean} [autoAbortFailure] boolean to indicate if an in progress upload should be aborted automatically upon failure. If not set, | ||
| * failures will be automatically aborted. |
There was a problem hiding this comment.
upload session will be automatically aborted
There was a problem hiding this comment.
Updated, thanks @frankyn
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕