search

axman6 / amazonka-s3-streaming

Provides a conduit based interface to uploading data to S3 using the Multipart API
MIT License
20 stars 23 forks source link

Rewrite all the things #25

Closed axman6 closed 1 year ago

axman6 commented 2 years ago

@endgame, @domenkozar, @koterpillar, @mpickering Hello!

So I decided that while Iw asn't studying I would actually try to bring together all the changes you'd proposed over the... years 😬 This branch includes work from all of you, thanks for your contributions and my deepest apologies for not responding sooner - I have never actually used this library in anger (and it scares me that other people are, but that's another issue!)

I would love it if you can take a look at the changes here, hopefully it cleans things up a whole bunch.

ghost commented 2 years ago

Thanks for testing @jhrcek, I have a feeling I may not have pushed my fix for the bug you've run into above (I realised that not all data being passed in was being sent in the original PR). Good points about the extensions, I'll remove the use of those - I just got so used to pretty imports in DAML 🥲

axman6 commented 2 years ago

Ok, I need to call a bike shed colour meeting. Currently, error handling in this code is a mess. There's a few ways things can fail and I don't know what the consistent way to deal with that actually is. Since I'm happy with breaking changes, is this actually an appropriate return type for streamingUpload

ConduitT ByteString Void m (Either (AbortMultipartUploadResponse, SomeException) CompleteMultipartUploadResponse)

I'm not sure what the ideal way to get errors to the user is in a way they can meaningfully deal with them. There's a whole range of ways to deal with errors in the upload, from always cancelling the multipart upload, to returning some metadata about what has been sent so that someone could reasonably restart an upload. I'm not sure how to deal with errors from Amazonka, and also internal invariants being violated - an example is https://github.com/axman6/amazonka-s3-streaming/pull/25#discussion_r760159555 above where I don't think it's possible for Amazonka to return to us a value which isn't Just but Amazonka's encoding allows for thew possibility.

So, I want to hear from users what you'd like to see done.

axman6 commented 2 years ago

@jhrcek If you have some time, can you try the commits I've pushed and make sure that the data is sent correctly now?

endgame commented 2 years ago

Bikeshed: Blue edition

What can happen?

I think you accept that you're providing a simple interface, and if something goes wrong the best you can do is attempt to gracefully abort the multipart upload. I think your error type will be something like:

data UploadResult
  = CreateMultipartUploadFailure _ -- ^ Didn't even create the multipart upload
  | PartUploadFailure _ -- ^ Part upload failed, aborted successfully. Include the AbortMultipartUploadResponse?
  | AbortMultipartUploadFailure _ -- ^ Part upload failed, but then the AbortMultipartUpload also failed
  | Success -- ^ Everything went OK. Do you return only the CompleteMultipartUploadResponse, or the responses from teh create/upload part?

And then your conduit is: ConduitT ByteString Void m UploadResult.

jhrcek commented 2 years ago

Good points about the extensions, I'll remove the use of those - I just got so used to pretty imports in DAML smiling_face_with_tear

If you want to support building with multiple GHC versions, it might make sense to setup CI. I see this repo has what seems to be no-longer-working travisCI setup. Nowadays it's pretty easy to setup github actions based CI. haskell-ci has good support for generating the github actions configs, but it's pretty easy to set it up by hand too (see e.g. https://kodimensional.dev/github-actions). If you want, I can give you a hand setting it up.

jhrcek commented 2 years ago

Hello @axman6 are there still some comments in this PR you would like to address or is it good to merge? Can I give you a hand to move this forward?

jhrcek commented 2 years ago

Hello @axman6 friendly ping about potentially merging this. Amazonka 2.0 might be released soonish, so it would be nice to potentially merge / publish this update if you'd like to get that in before switching to amazonka 2. Any concerns I can help you resolve?

codygman commented 2 years ago

This should be closed because the changes were merged in #28 right?

axman6 commented 1 year ago

I'm going to close this issue, as most of what's been mentioned here has now been included int master.

Since this will ping several people who are stakeholders in this project, I thought I'd mention that master has been updated to work against the RC2 release of amazonka (thanks @jhrcek!) so you might want to update (or at least update your references to amazonka - no changes were needed on to this library luckily.

Given the hopefully imminent release of amazonka-2.0, I'd appreciate if you'd give the library a bit of a test and let me know if there are any changes you'd like to see before that release and subsequent release of our 2.0. @domenkozar I know you've been using this for some serious projects so would particularly appreciate your input.