Skip to content
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

PB-615: Spec multipart upload process #425

Merged
merged 3 commits into from
Jul 2, 2024

Conversation

benschs
Copy link
Contributor

@benschs benschs commented Jul 1, 2024

Describe multipart upload process in more details. Added a quick description of the process. After talk with @boecklic we will add more detailed documentation (probably in another tool) later.

Describe multipart upload process in more details.
@benschs benschs force-pushed the feat-PB-615-spec-multipart-upload branch from 120709d to 196b78d Compare July 1, 2024 13:10
Update the supported media type list in the spec to match the actual
implementation. Add mode information to the delete feature request description.
Copy link
Member

@adk-swisstopo adk-swisstopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think that's exactly the kind of high-level description we are missing.

If any errors happen during the upload process, you can [abort the upload](#tag/Asset-Upload-Management/operation/abortMultipartUpload)
and restart the process.

There can only be 1 upload in progress per asset. If you wish to start a new upload you must first abort any existing upload that is in progress.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bit unclear. From my understanding it's OK to upload multiple parts concurrently but you cannot "overwrite" (start a new upload of) an asset for which upload has not completed (or been aborted).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Esp. the flow chart could be of interest to some users

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of that doc. Maybe it should be pulled into the API doc itself instead of being a separate document? If it needs to remain separate, I strongly support linking to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adapted the concurrent upload part sentence. I was also not aware of this document. I added the detailed description of the upload to the spec, as well as the diagram asset to the static files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest replacing the document with a redirect to the API doc that has the content. Less copypasta to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I removed the existing file as all the content is integrated in the spec. Also moved both diagram files to the static assets.

</ol>

Any file that is larger than 5 GB must be split into multiple parts. A file part must be at
least 5 MB except for the last one and at most 5 GB, otherwise the complete operation will fail.
Copy link
Member

@hansmannj hansmannj Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "except for the last (or only) one" ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to call it out, I would add a separate sentence instead of parenthesis. "If there is only one part, it can be smaller than 5 MB since it is also the last part."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following this sentence there is the following sentence:
"If the file is less that 5 GB, you will only upload a single part, but must still start and complete the process as with multiple parts."
Do you think it needs more than this?

Copy link
Member

@hansmannj hansmannj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM 👍

description: Use this method to delete an existing feature/item.
description: |
Use this method to delete an existing feature/item. The feature can only be deleted if all
its assets have been deleted first. Also see [Delete an existing asset by id](#tag/Data-Management/operation/deleteAsset).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"See also" sounds less weird than "Also see" I think.

Detailed description of upload process and error handling.
Moved from existing files found in doc/
@benschs benschs force-pushed the feat-PB-615-spec-multipart-upload branch from f28886f to 8773def Compare July 2, 2024 08:04
@benschs benschs merged commit d4d5bdb into develop Jul 2, 2024
3 checks passed
@benschs benschs deleted the feat-PB-615-spec-multipart-upload branch July 2, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants