opened
jcmfernandes
username_0
This PR adds support for chunked uploads. Box suggests using these when uploading large files. It's possible to upload multiple chunks in parallel.

Important:
- Aborting sessions in case of an error works on a best-effort basis
- Test file `large test file.txt` is indeed large, but git compresses packfiles with gzip, so it takes only 50kB in the repository
Add support for chunked uploads
issue
created
xhocquet
username_1
Hey @username_0 , thanks for the contribution! This would be a great feature to add and it looks like you've put in a lot of work 😄 Thanks in advance for the spec as well!

In general I'm happy with the implementation and understand the necessity for the tons of new methods, however I'd like to organize this new code a little bit. Maybe a new `chunked_uploads` file? 

Another thought I had was that I don't love adding a new dependency. I'm currently finishing up a branch to remove the `addressable` dependency (100 kb just for simple URI building), but this library is only 9kb, so that's pretty nice.
comment
created
jcmfernandes
username_0
Hey @username_1, thank you for your work in this library! It's always great to be able to give something back to the community. 🙌 

Regarding moving these methods to its own file, granted, I agree and will do it.

About adding a new dependency, I did foresee it not being consensual. Given that I believe that rolling out my own parallel map here wouldn't be wise, I see 3 options moving forward:

1) add gem `parallel` as a required dependency (i.e., leave the PR as is)
1) make gem `parallel` a runtime dependency, raising an error when one calls `Boxr.chunked_upload_file` with `n_threads > 1` (adding this to the documentation, obviously)
1) remove support for parallel uploads (I would prefer not going with this one...)

What do you think?
comment
created
xhocquet
username_1
@username_0 #2 sounds interesting to me, but I was not able to find docs that support such functionality with rubygems. Could you please point me to some docs?
comment
created
jcmfernandes
username_0
It will look something like

```ruby
def chunked_upload_to_session_from_io(io, session, n_threads: 1, content_created_at: nil, content_modified_at: nil)
  content_ranges = []
  offset = 0
  loop do
    limit = [offset + session.part_size, io.size].min - 1
    content_ranges << (offset..limit)
    break if limit == io.size - 1

    offset = limit + 1
  end

  parts =
    if n_threads > 1
      raise BoxrError.new(boxr_message: "parallel chunked uploads require gem parallel (~> 1.0) to be loaded") unless gem_parallel_available?

      Parallel.map(content_ranges, in_threads: n_threads) do |content_range|
        File.open(io.path) do |io_dup|
          chunked_upload_part_from_io(io_dup, session.id, content_range)
        end
      end
    else
      content_ranges.map do |content_range|
        chunked_upload_part_from_io(io, session.id, content_range)
      end
    end

  commit_info = chunked_upload_commit_from_io(io, session.id, parts,
    content_created_at: content_created_at, content_modified_at: content_modified_at)
  commit_info.entries[0]
end

def gem_parallel_available?
  gem_spec  = Gem.loaded_specs['parallel']
  return false if gem_spec.nil?

  Gem::Requirement.new('~> 1.0').satisfied_by?(gem_spec.version) && defined?(Parallel)
end
```
comment
created
xhocquet
username_1
@username_0 Very cool! I love this idea since it adds a new, optional requirement to use a new, optional feature. 

I'd also want a prominent note in the README with a link to the library as well as an explanation of this implementation.
comment
created
jcmfernandes
username_0
Hey @username_1, I think I'm done here. I took the liberty of updating the links to box's documentation, as they recently (last week?) updated their public API docs. The existing links still work, but, for example, point to an old page that doesn't contain information about chunked uploads.

What do you think?
comment
created
xhocquet
username_1
@username_0 Thanks for putting in the extra effort around the docs, I love it! 

I gave the spec a run on my machine and wasn't able to complete a chunked upload, so I'm going to give it some more tries and make sure there's no issues before merging. 

We'll also want to bump the minor version for a new feature, but no breaking changes! 🎉
comment
created
xhocquet
username_1
I ran a test locally with two threads successfully 🎉 So this has my ok to merge once the version is bumped. Docs and tests are present! 👍
comment
created
jcmfernandes
username_0
Awesome @username_1 ! 😄 🎉
comment
created
xhocquet
username_1
Hey @username_0 , just a reminder in case you forgot! If you want to bump the minor version, I'll merge! Thanks 🙏
comment
created
jcmfernandes
username_0
Hey @username_1, sorry about that. There was a minor misunderstanding here, as I thought that would want to do that in separate PR. It's done 😄 let me know once you release it on rubygems. Thank you! 🙌
comment
created
xhocquet
username_1
@username_0 No problem! I guess it's a slightly weird way of doing things! 😛
comment