Multipart enhancement - Githubissues

irods / irods_client_s3_api

C++ S3 API for iRODS

BSD 3-Clause "New" or "Revised" License

0 stars 5 forks source link

Multipart enhancement #119

Closed JustinKyleJames closed 3 weeks ago

JustinKyleJames commented 1 month ago

This is the for multipart enhancement described in https://github.com/irods/irods_client_s3_api/issues/114.

I have also implemented AbortMultipartUpload.

Notes:

Tests are passing.
I had previously made all of the changes for PR #115. I opened a new PR simply because so many changes have been made since that PR was opened.
This is now working for both normal clients and clients that use chunked encoding.
I made a couple of minor updates not directly related to this enhancement:
- I changed FUNCTION references to func. This was only for these two files that are touched in this PR.
- I changed some logging from debug to trace.

A couple of other concerns/questions:

The entry in shared memory is cleaned up when CompleteMultipartUpload or AbortMultipartUpload returns. However, if neither is called it won't be cleaned up until the server restarts. A new call to InitiateMultipartUpload would generate a new upload_id so that upload should behave correctly.

trel commented 1 month ago

The entry in shared memory is cleaned up when CompleteMultipartUpload or AbortMultipartUpload returns. However, if neither is called it won't be cleaned up until the server restarts.

Should we consider cleaning up the entry in shared memory if a certain amount of time passes? Aka a timeout on idle shared memory entries? If so... what is a safe amount? An hour of no activity? A day? A week?

JustinKyleJames commented 1 month ago

The entry in shared memory is cleaned up when CompleteMultipartUpload or AbortMultipartUpload returns. However, if neither is called it won't be cleaned up until the server restarts.

Should we consider cleaning up the entry in shared memory if a certain amount of time passes? Aka a timeout on idle shared memory entries? If so... what is a safe amount? An hour of no activity? A day? A week?

I think the ListMultipartUpload could return those and CancelMultipartUpload already cleans this up so I think that would be sufficient.

JustinKyleJames commented 1 month ago

very impressive. glad this is behaving.

we have existing tests? new tests?

For the most part the existing tests should suffice. I did have a problem with missing the last close which would cause a second put to that object to fail if done too quickly. I just updated the tests to follow up the first large file put with a second one for each of the clients.

JustinKyleJames commented 3 weeks ago

Is this ready to be squashed?

korydraughn commented 3 weeks ago

Yes, proceed with squashing if it's passing the tests.

JustinKyleJames commented 3 weeks ago

Yes, proceed with squashing if it's passing the tests.

I have squashed.

Note that I also corrected my .vimrc file which was no longer highlighting spaces at the end of lines. I noticed some of these in my docker-compose.yml so removed those. Other than that this is identical to what you reviewed.

korydraughn commented 3 weeks ago

Please capture the design/ideas behind the multipart enhancements in the commit body of 45508f5.

JustinKyleJames commented 3 weeks ago

Please capture the design/ideas behind the multipart enhancements in the commit body of 45508f5.

I added the design in the commit message.

alanking commented 3 weeks ago

D'oh. I misread the request for the design description. Looks good