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
Add OpenChunkWriter and ChunkWriter interfaces and refactor s3.uploadMultpart and multiThreadCopy to use them #7154
Conversation
@ncw FYI: @vitorog is my work colleague, we are working together to get this done. You might also see commits from @AffDNeto and @sysedwinistrator :) Tomorrow we will try to do the same thing @vitorog did here for S3 on another provider, probably GCS, to further validate the interface. |
After looking at the code a bit, I saw that the way gcp does multipart is basically by implement the S3 API, and this is already supported by configuring a GCS bucket to use S3, so I'll leave that out for now |
If we can work out whether we need an I've just done the 1.63 release so this is a great time in the dev cycle for experimental code! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking nice! I put a few comments inline.
We should probably sort out exactly what goes in which commit at some point but that can wait until the end easily enough.
backend/local/local.go
Outdated
if err != nil { | ||
return -1, errors.New("failed to write chunk") | ||
} | ||
fs.Debugf("", "wrote chunk %v with %v bytes", chunkNumber, n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its probably worth caching the result of src.Remote()
so you can use it here - this will make the logs much easier to read!
fs.Debugf(w.remote, ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
backend/local/local.go
Outdated
} | ||
|
||
func (w *localChunkWriter) Abort() error { | ||
// TODO: Is just closing enough? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to delete the file - that is what s3 etc will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the code to delete the file.
backend/local/local.go
Outdated
@@ -1374,7 +1374,7 @@ type localChunkWriter struct { | |||
} | |||
|
|||
func (w *localChunkWriter) WriteChunk(chunkNumber int, reader []byte) (int, error) { | |||
offset := int64(chunkNumber) * w.chunkSize | |||
offset := int64(chunkNumber-1) * w.chunkSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably start the chunks from 0 otherwise we'll be confusing all the 0-based Go programmers until the end of time ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's probably a good idea 😅
Sorry, I was thinking on the S3 API while implementing this (where part numbers start with 1).
I updated the code to start chunks from 0.
Are we going to remove support for Just thinking about how this would work for the local backend. It would use more memory with more buffers, but it would probably leave the files less fragmented as it would be writing small chunks but all close to each other which the OS is likely to buffer in RAM. So as far as local backend is concerned this is probably an improvement. So we should ditch the I note that the conversion from OpenWriterAt to ChunkWriter is generic - I say this because there is a PR in the pipeline adding OpenWriterAt to the smb backend so it would be nice not to re-write it there! So you could make the code in local just instantiate an adaptor to implement the interface.
Multithread copies can't do unknown sized copies at the moment so this needs a little care. I guess what we are factoring out is not the entire multithread copying routine but just the guts of the moving chunks around. To think about - memory management mmap, memory pools etc. We currently manage this for the s3 etc backends and it is quite important, so we need the same controls in the operations multithread routines. Though we could turn Note also we have two uses of multithread copy in the s3 backend, one for multipart uploads and the other for server side copies of large files. They use pretty much the same code structure but don't need to move actual bytes. I don't know if this can be factored out - probably not. |
IMHO reader is a better choice:
|
Yes, noted.
There are two reasons the SDK needs to seek
So I suspect If we were to implement this naively by seeking an incoming data stream then (which is fairly easy - rclone has a way of opening objects as So I think this is likely going to need to be a memory backed buffer anyway. However at some point I'd like to switch to more of a scatter gather memory buffer with a lot of 1MB pages say which will would fit well within the
Yes. Though see above re retries! I had a look to see what the SDKs are expecting
So I think |
Damn, I keep forgetting about the retries. So I guess you are right, |
hi, @ncw @jorjao81 , based on the discussion, I refactored the ChunkWriter interface to use an io.ReadSeeker. I also updated the multi-thread copy to use the new interfaces, based on @jorjao81 's PR in #7061 and fixed the build and lint errors (I think).
@ncw I implemented an adapter (openChunkWriterFromOpenWriterAt) in multithread.go. From my understanding, with this adapter we don't even need to implement OpenChunkWriter in the local backend, right? |
69ce928
to
7a05ca0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look through the OpenWriterAt adapter - looks great :-)
fs/operations/multithread.go
Outdated
} | ||
|
||
func (w writerAtChunkWriter) Close() error { | ||
return nil // NOP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this need to close the w.writerAt
since we opened it in openChunkWriterFromOpenWriterAt
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, thanks. I updated the PR.
fs/operations/multithread.go
Outdated
return obj.Remove(w.ctx) | ||
} | ||
|
||
func openChunkWriterFromOpenWriterAt(openWriterAt func(ctx context.Context, remote string, size int64) (fs.WriterAtCloser, error), writeBufferSize int64, streams int, f fs.Fs) func(ctx context.Context, src fs.ObjectInfo, options ...fs.OpenOption) (chunkSizeResult int64, writer fs.ChunkWriter, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That function definition is a bit of a mouthful! Perhaps we should define some aliases in fs/features.go
. Anyway it is fine so don't do that for the moment - that can be a job for another day!
Great.
I've re-ran the CI so you can check in a moment.
Your adapter looks great. I made a couple of comments above but it is the right approach definitely. And we don't need the |
Thanks for the review and comments @ncw . After running some tests, I noticed an issue with the OpenChunkWriter interface:
I'm asking this because the multiThreadCopy function is defined as:
where the remote refers to the destination file. If we use "src.Remote()" in OpenChunkWriter, it will only work if the destination file has the same name (remote) as the source (otherwise it fails with |
Traditionally you'd use However making it line up with the multi thread copy code makes it slightly easier to use, so adding a |
Executed some tests in a m6in.8xlarge EC2 instance (network bandwidth of 50 Gbps).
*250-threads-250M-chunk-size: crashed with an "out of memory" error Transfer of the same file (single threaded):
|
hi, @ncw, after running more tests, I noticed an issue related to this setModTime call: For S3, the setModTime implementation (https://github.com/rclone/rclone/blob/master/backend/s3/s3.go#L5130) makes a copy of the object over itself. However, after doing a multi-threaded copy we don't need to do this, since the metadata should already be correct. Another issue is related to the Accounting. I couldn't find a better way of integrating it with the OpenChunkWriter. |
keep up the good work guys, looking forward to these parallel chunked download, upload interfaces. In chunked upload interfaces: please also consider the interface to list and use meta information of already uploaded parts. This can be used to resume multipart upload after rclone is restarted for whatever reason and its smart to skip uploading parts already upload. Example: #7189 |
Amazing performance :-) Is 2.5 GiB/s network saturated? As we are doing down and up or would 5GiB/s be network saturated?
Yes, this SetModTime is left over from copying to the local file system where it is necessary.
I think avoiding the SetModTime at the end if the Fs we are copying to does not have the I think your patch is probably OK too though!
What we want is something like this rclone/fs/accounting/accounting.go Line 564 in 4444037
That wraps an I'd be happy to leave a FIXME in the code and address this later if you want. |
I think 50 Gbps is the aggregate bandwidth: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/monitoring-network-performance-ena.html
I reverted my previous commit and implemented it as you said.
I tried that approach, but I couldn't make it work correctly, I think because of the seeks+reads. Finally, I had to add a new flag "multi-thread-chunk-size" to set the chunk size for the "openChunkWriter with writerAt" adapter. Do you think we could merge this PR? If yes, should I rebase/squash some of the commits? |
I think from reading the doc that yes we are saturating the network :-)
👍
That's fine. Can't fix all of the world's problems in one PR :-)
I think we are heading towards merge yes :-) Can you rebase off master and then squash into logical changes like the plan we originally agreed. That would be perfect :-) I think what I'd like to do then is pull this locally for the last review. I'll fix up any little things I notice when doing the final review if that is OK with you? Then I'll merge. We can then work on the next bits! |
… available rclone#7056 If the feature OpenChunkWriter is not available, multithread tries to create an adapter from OpenWriterAt to OpenChunkWriter.
Sounds awesome, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you - this code is looking excellent :-)
I will merge this now.
Do you want to work on the final part of this
Refactor multipart upload in s3 to use version in operations
In another PR?
Or I can have a go at that if you want.
1.64 is going to be a very good release :-)
Thanks, @ncw !
I can give it a try (though I could use some pointers). |
I had a look at what this would involve and I think this bit isn't properly thought through yet! I will have a go with it, but I think it is going to be more complicated than I first thought!
I'd be interested to see what that looks like. Note that there are some unmerged patches for resuming uploads in the backlog... |
Wait, does this work with every backend, or only S3 ? Good work either way ! EDIT : Only S3 it seems, upload to Dropbox still using only one stream/connex. Sorry I misunderstood the commit. |
As I understand, this PR creates an interface and implements it for S3 only (and for things that implement OpenWriterAt, probably only filesystems like local and the SMB backend). As other backends implement the interface, they will also get the capability. This has to be done one by one; and of course requires the backend provider to even support it (I looked into NetStorage because that would be a use case for us, but apparently there is no way). |
There is more to do on this - I'm going to discuss on #7056 with some things to test :-) |
What is the purpose of this change?
Following the discussions in #7061 and #7056 this change is meant to pave the way for fast parallel transfers between remotes.
It defines the interfaces suggested by @jorjao81 and implements them for the S3 and Local backend.
Also, the s3.uploadMultipart logic was refactored to use OpenChunkWriter and ChunkWriter (s3 multi part uploads should behave the same as before).
In https://github.com/rclone/rclone/pull/7061 - @ncw suggested the following roadmap:
Therefore, the next steps would be:
Looking forward to some feedback. I'm not experienced with Go, so apologies for any (dumb) mistake. 😅
Was the change discussed in an issue or in the forum before?
#7061
#7056
Checklist