[Proposal] OpenAPI for S3 multipart upload #7197

nopcoder · 2023-12-23T08:55:38Z

No description provided.

github-actions · 2023-12-23T08:55:51Z

No linked issues found. Please add the corresponding issues in the pull request description.
Use GitHub automation to close the issue when a PR is merged

github-actions · 2023-12-23T09:06:14Z

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed

Thanks!

Let's talk about the client having to specify the number of parts in advance.

design/open/openapi-multipart-upload.md

arielshaqed · 2023-12-24T08:55:52Z

design/open/openapi-multipart-upload.md

+
+In order to minimaize the scope of this feature and understand the benefits before supporting all blockstores, this feature will be scoped to S3 blockstore and only if presigned capability is enabled.
+
+Initiation of Upload: The API will allow clients to initiate a multipart upload session, assigning a unique upload ID for subsequent operations. The client will require pass the number of parts it requires and the call will provide a set of URLs to upload each part.


AFAICT S3 multipart does not require the client to specify the number of parts in advance.

This is actually useful in some cases 🤷🏽! For example, say I want to add multipart to lakeFSFS. The Hadoop FS API is basically create, write, write, ..., close. And indeed S3A keeps a current block in memory and uploads it as a part whenever it fills. This is actually probably efficient. But it relies on not having to supply the intended number of blocks when starting an upload.

Agree, any streaming or working with unknown size is good for multipart or at least working with "upload manager" that will perform multipart or single object upload.

In order to limit the scope of the implementation, it is a limitation as I didn't want the client to call lakeFS for each part.
Working with local file and known in advance the size we like to upload - optional list as part of the create to start multipart will handle it on a single request.
The suggested API doesn't require the server to return any presigned part so we can include this capability later without breaking the API.

OK, so perhaps document next possible step -- it isn't clear?

For instance somewhere...

### Next steps None of the returned URLs has to be used, it is fine to ask for more than are needed. In future we may add an _additional_ API call to URLs for uploading more parts. This will allow more "streaming" uses, for instance as parallels to how Hadoop S3A uses the S3 MPU API. The current API is compatible with such an addition.

arielshaqed · 2023-12-24T08:57:14Z

design/open/openapi-multipart-upload.md

+
+Uploading Parts: Clients can upload individual parts of the file in parallel or in sequence. Each part will be based on the presigned URL provided by the initial call.
+
+Support for Large Files: The API will handle files of substantial sizes, ensuring that large datasets can be uploaded without issues. Minimum part size will be 5M.


Minimum part size will be 5M.

This sounds suspiciously like the S3 limitation -- an abstraction leak. So if we expose this, it should at least be the minimum part size of {S3, GCS, Azure BlobStore}.

I'll update that each part (exclude the last one) minimum size depends on the underlying blockstore implementation.

design/open/openapi-multipart-upload.md

arielshaqed · 2023-12-24T08:58:09Z

design/open/openapi-multipart-upload.md

+
+**Paths for multipart upload operations**
+
+```yaml=


Suggested change

```yaml=

```yaml

Thanks - my markdown editor use yaml= to render yaml + line numbers

arielshaqed · 2023-12-24T09:01:12Z

design/open/openapi-multipart-upload.md

+        locations:
+          type: array
+          items:
+            $ref: "#/components/schemas/StagingLocation"


Obviously if we don't require the client to supply the number of parts in advance, then we cannot do this. We will need another API call to get another presigned URL for an upload ID.

True, in the current spec the number of parts in the request is optional. But will be required by the server.
And the server response include StagingLocations as optional in the case we will like to support dynamically request for part presigned URL(s).

arielshaqed · 2023-12-24T09:02:29Z

design/open/openapi-multipart-upload.md

+        parts:
+          type: array
+          items:
+            $ref: "#/components/schemas/UploadPart"


Perhaps worth mentioning in the description that the object is created of these parts in the specified order -- NOT in the order in which the objects were uploaded, or in the order that createMultipartUpload returned the parts.

Good point - the parts should be sorted by part number.

Amusingly, the S3 API does not document such a requirement AFAICT? But this is fine.

Yea, you learn from the hard way.

Error Code: InvalidPartOrder Description: The list of parts was not in ascending order. The parts list must be specified in order by part number. HTTP Status Code: 400 Bad Request

Documented in https://docs.aws.amazon.com/cli/latest/reference/s3api/complete-multipart-upload.html

arielshaqed · 2023-12-24T09:04:10Z

design/open/openapi-multipart-upload.md

+        part_number:
+          type: integer
+          minimum: 1
+          maximum: 1000


S3 allows 10_000 parts. In any case I do not believe that this limitation should be in the OpenAPI spec: if I later decide to change it on lakeFS, old clients should be able to use this higher maximum value.

arielshaqed · 2023-12-24T09:05:12Z

design/open/openapi-multipart-upload.md

+    - **Minimum part size:** Each part must be at least 5MB in size. This is a temporary constraint and isn't currently configurable or discoverable. It will become an option when additional storage options are supported.
+- **Initiating the upload:** When starting a multipart upload, you'll need to specify the total number of parts in your request. It reduce the requests for each part presigned URL when the client already knows the size.
+- **Presigning part:** Presigned of a specific part URL for upload will not be supported. This will block unknown size upload using this API.
+- **Limited support:** Multipart uploads are not currently supported.


Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

- Change from 1000 to 10000 parts limit - Remove the limit from the open api spec

Replace yaml= to yaml

limited support

nopcoder · 2023-12-26T20:59:29Z

Thanks!

Let's talk about the client having to specify the number of parts in advance.

Added this one as limitation, didn't want to open another API which I will not use.
Adding the new api "request for part(s) presign URLs" will not break or change the current API.

eladlachmi

This proposal is on point.
This needs to be part of the API! ✌🏻
I added a few minor comments.

design/open/openapi-multipart-upload.md

eladlachmi · 2023-12-27T12:53:16Z

design/open/openapi-multipart-upload.md

+**Paths for multipart upload operations**
+
+```yaml
+  /repositories/{repository}/branches/{branch}/staging/multipart:


If we're planning to support both pre-signed and local credentials eventually, it would make sense to add a presign query param here to align with other endpoints. Initially, we can set its default to true and return HTTP status 400 (or 405) if it's set to false. Obviously, this limitation would need to be documented.

Good point!

Didn't want to open another interface to upload data through lakefs

The requests to create/complete/abort multipart works on upload ID that represents the operation. The presined parts currently returned on CreateMultipartUpload operations only if you specify the number of parts you like to get and it is optional. It means that if we like in the future to open upload part data through lakeFS we can still provide this API.

I will update the spec to rename parts to presigned_parts to indicate this number represents the number of urls the call will return.

I don't see the alternative as uploading through lakeFS. I see the non-pre-signed version of this using local credentials on the client, i.e., the client uses the AWS SDK to upload directly to an S3, according to a physical address returned from this endpoint.

But I'm also ok with not doing this now and seeing if there's a need there.

Both ways - using presign and non-presigned URLs require local credentials to work.
Supporting multipart upload using the API while data transfer goes though lakeFS is possible, but I would like to avoid it and just provide more ways to enable the client direct access to the specific object using the underlying object store.

design/open/openapi-multipart-upload.md

arielshaqed

Thanks!

design/open/openapi-multipart-upload.md

arielshaqed · 2023-12-27T15:30:56Z

design/open/openapi-multipart-upload.md

+
+In order to minimaize the scope of this feature and understand the benefits before supporting all blockstores, this feature will be scoped to S3 blockstore and only if presigned capability is enabled.
+
+Initiation of Upload: The API will allow clients to initiate a multipart upload session, assigning a unique upload ID for subsequent operations. The client will require pass the number of parts it requires and the call will provide a set of URLs to upload each part.


OK, so perhaps document next possible step -- it isn't clear?

For instance somewhere...

### Next steps None of the returned URLs has to be used, it is fine to ask for more than are needed. In future we may add an _additional_ API call to URLs for uploading more parts. This will allow more "streaming" uses, for instance as parallels to how Hadoop S3A uses the S3 MPU API. The current API is compatible with such an addition.

arielshaqed · 2023-12-27T15:33:00Z

design/open/openapi-multipart-upload.md

+        parts:
+          type: array
+          items:
+            $ref: "#/components/schemas/UploadPart"


Amusingly, the S3 API does not document such a requirement AFAICT? But this is fine.

Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com>

nopcoder · 2023-12-30T13:09:02Z

Thanks!

Thanks about the multipart upload complete api parts order, the response in the previous comment, I'll paste it here too:

Error Code: InvalidPartOrder

Description: The list of parts was not in ascending order. The parts list must be specified in order by part number.
HTTP Status Code: 400 Bad Request

Documented in https://docs.aws.amazon.com/cli/latest/reference/s3api/complete-multipart-upload.html

eladlachmi

LGTM!

design/open/openapi-multipart-upload.md

N-o-Z · 2023-12-31T10:54:32Z

design/open/openapi-multipart-upload.md

+### Support and discover
+
+Presign support is a capability lakectl discover before switching to use presign for upload or download from lakeFS.
+The multipart upload support will be part of the storage capability add `multipart_upload_support` optional field that when set to `true` the user can perform multipart upload using the new API.


Why do we actually need this extra param?
Can't we rely on the block adapter type for now?

It means that lakectl as a client will need to check:

s3 block adapter

presgined support enabled

lakefs version is above v1.x that include this feature

For 1,2 the client can get this information from the GetConfig API
Regarding no. 3 I think is not necessary, IMHO it's perfectly fine for the server to return 501 and for the client to handle it appropriately.
Adding this field will cause ambiguity, since it is a direct result of blockstore_type + pre_sign

I took elad's suggestion and like a feature flag it represents the capability. if we like to support multipart through lakefs API we can.

The second issue related to try and fail does not help with client side code when you like to sync files in parallel while trying to understand one of you can use this feature. possible but not clean.
Also status code for unsupported operation in the logs vs clean 200 ok, I prefer the last.

design/open/openapi-multipart-upload.md

N-o-Z

Very cool design,
One question regarding the new config param

Co-authored-by: N-o-Z <ozery.nir@gmail.com>

N-o-Z

Approved thanks!

[Proposal] open api for s3 multipart upload

ba8e8b1

nopcoder requested a review from a team December 23, 2023 08:55

nopcoder self-assigned this Dec 23, 2023

nopcoder added the proposal label Dec 23, 2023

arielshaqed requested changes Dec 24, 2023

View reviewed changes

nopcoder and others added 6 commits December 24, 2023 11:09

Update design/open/openapi-multipart-upload.md

d2b3aca

Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

Update openapi-multipart-upload.md

decdb18

- Change from 1000 to 10000 parts limit - Remove the limit from the open api spec

Update openapi-multipart-upload.md

f551a9f

Replace yaml= to yaml

Update openapi-multipart-upload.md

c9d7281

limited support

Update openapi-multipart-upload.md

ffcaa85

Update openapi-multipart-upload.md

4e7aaa9

nopcoder changed the title ~~[Proposal] open api for s3 multipart upload~~ [Proposal] OpenAPI for S3 multipart upload Dec 25, 2023

nopcoder requested a review from arielshaqed December 26, 2023 20:59

eladlachmi reviewed Dec 27, 2023

View reviewed changes

arielshaqed approved these changes Dec 27, 2023

View reviewed changes

nopcoder and others added 2 commits December 30, 2023 14:58

Update design/open/openapi-multipart-upload.md

cbcc210

Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

Update design/open/openapi-multipart-upload.md

9a9f834

Co-authored-by: eladlachmi <110764839+eladlachmi@users.noreply.github.com>

nopcoder added 4 commits December 30, 2023 15:22

Update openapi-multipart-upload.md

d8f3d4a

Update openapi-multipart-upload.md

bef0f2a

Update openapi-multipart-upload.md

f3943fe

Update openapi-multipart-upload.md

a430ef6

nopcoder requested a review from eladlachmi December 30, 2023 13:40

eladlachmi approved these changes Dec 31, 2023

View reviewed changes

N-o-Z reviewed Dec 31, 2023

View reviewed changes

design/open/openapi-multipart-upload.md Outdated Show resolved Hide resolved

N-o-Z reviewed Dec 31, 2023

View reviewed changes

design/open/openapi-multipart-upload.md Outdated Show resolved Hide resolved

N-o-Z reviewed Dec 31, 2023

View reviewed changes

design/open/openapi-multipart-upload.md Outdated Show resolved Hide resolved

N-o-Z requested changes Dec 31, 2023

View reviewed changes

nopcoder and others added 3 commits December 31, 2023 16:07

Update design/open/openapi-multipart-upload.md

c8c140c

Co-authored-by: N-o-Z <ozery.nir@gmail.com>

Update design/open/openapi-multipart-upload.md

a1ad94f

Co-authored-by: N-o-Z <ozery.nir@gmail.com>

Update design/open/openapi-multipart-upload.md

dc0cc19

Co-authored-by: N-o-Z <ozery.nir@gmail.com>

nopcoder requested a review from N-o-Z December 31, 2023 14:18

N-o-Z approved these changes Jan 1, 2024

View reviewed changes

move to approved

571566d

nopcoder enabled auto-merge (squash) January 1, 2024 22:16

nopcoder disabled auto-merge January 1, 2024 22:16

nopcoder added exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached labels Jan 1, 2024

nopcoder enabled auto-merge (squash) January 1, 2024 22:17

nopcoder merged commit 9b13af2 into master Jan 1, 2024

nopcoder deleted the proposal/openapi-s3-multipart branch January 1, 2024 22:34


		In order to minimaize the scope of this feature and understand the benefits before supporting all blockstores, this feature will be scoped to S3 blockstore and only if presigned capability is enabled.

		Initiation of Upload: The API will allow clients to initiate a multipart upload session, assigning a unique upload ID for subsequent operations. The client will require pass the number of parts it requires and the call will provide a set of URLs to upload each part.


		Uploading Parts: Clients can upload individual parts of the file in parallel or in sequence. Each part will be based on the presigned URL provided by the initial call.

		Support for Large Files: The API will handle files of substantial sizes, ensuring that large datasets can be uploaded without issues. Minimum part size will be 5M.

[Proposal] OpenAPI for S3 multipart upload #7197

[Proposal] OpenAPI for S3 multipart upload #7197

Conversation

nopcoder commented Dec 23, 2023

github-actions bot commented Dec 23, 2023

github-actions bot commented Dec 23, 2023 • edited Loading

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder commented Dec 26, 2023

eladlachmi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder commented Dec 30, 2023

eladlachmi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z left a comment

Choose a reason for hiding this comment

N-o-Z left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 23, 2023 •

edited

Loading