Initial implementation of the new chunked upload #20118
Conversation
ghost
added
the
|
How does this stream work and still exist between PHP requests ? I see fseek is disabled. Maybe not implemented yet ? |
the AssemblyStream will assembly the chunk files while the file is moved/copied to it's final destination. There is no intent to have it available between requests. Up to now I don't see the need to seek |
|
I still don't understand. Is the AssemblyStream used for a single chunk then, and it would save each chunk as separate files like we did in the past ? So basically when uploading 3 chunks, the client does 3 PUT requests, each gets its own AssemblyStream that saves the chunk contents into a file. But then who puts the chunks together ? |
|
How does this fit with: I can't seem to infer this from the code. |
|
the move is like this - https://github.com/DeepDiver1975/dav/blob/master/tests/client/upload.php#L33 |
|
this magic .file file is implemented as this FutureFile which uses the AssemblyStream to stream all chunks to the destination. |
|
Okay, so is the AssemblyStream internally putting the chunks into that file at the right position, doing a fseek internally ? I suppose those "nodes" are actually the ranges ? (that's why I asked about fseek) |
DeepDiver1975
force-pushed
the
chunked-upload-dav
branch
from
a39573f
to
3bc02fc
Compare
|
So the AssemblyStream is actually simulating a single stream based on several chunk files, and is read by the MOVE operation (I thought AssemblyStream was for putting, not for reading, that's why I was confused). So the chunk files themselves simply get uploaded as if they were regular files. Did I get it correctly ? 😄 |
yes |
|
|
||
| use Sabre\DAV\IFile; | ||
|
|
||
| class AssemblyStream implements \Icewind\Streams\File { |
There was a problem hiding this comment.
Can you add a comment here to explain the AssemblyStream to avoid the many possible confusions ?
|
Cool stuff 😄 |
| class AssemblyStream implements \Icewind\Streams\File { | ||
|
|
||
| /** @var resource */ | ||
| private $context; |
There was a problem hiding this comment.
I'm reasonably sure this needs to be public
There was a problem hiding this comment.
@icewind1991 can you elaborate? I did not face any issues having this private
|
do we really want to do this for 9.0 ? Not sure we have the capacity |
|
|
||
| // sort the nodes | ||
| $nodes = $this->nodes; | ||
| @usort($nodes, function(IFile $a, IFile $b) { |
There was a problem hiding this comment.
ah - because of that:
usort(): Array was modified by the user comparison function
|
@karlitschek the code is ready since month. No reasons to wait any longer from my pov. |
|
did you already tried this with encryption enabled? Wonder how this works if uploaded chunks don't meet the expected chunk size of a encryption block? For the last chunk of a file encryption apply padding to meet the expected size. So maybe we will just end up with some additional padded chunks in the middle of the file. So it could work, just increase the size of the encrypted file a bit... but will fseek work on such a file? We need to test this in detail if not already happened. |
Nice input - thanks a lot! We need to add tests with encryption enabled. |
|
|
||
| list($node, $posInNode) = $this->getNodeForPosition($this->pos); | ||
| if (is_null($node)) { | ||
| return null; |
There was a problem hiding this comment.
not a string as indicated in PHP Doc
DeepDiver1975
force-pushed
the
chunked-upload-dav
branch
from
3bc02fc
to
88e35d4
Compare
| } | ||
|
|
||
| function createFile($name, $data = null) { | ||
| // TODO: verify name - should be a simple number |
There was a problem hiding this comment.
intend to complete this before merging?
|
Overall this looks nice and clean, though information is a bit sparse, as pointed out by @PVince81. |
|
Estimation: 2 Days |
The same intergration tests as for the old endpoint. But now using the new chunking. We upload 3 chunks in different order and the result should be the same in all three.
DeepDiver1975
force-pushed
the
chunked-upload-dav
branch
from
48ecdf1
to
bb2e68f
Compare
|
Unit and integration tests are passing - let's move this in |
|
For the record: We do not have the implementation on the client yet. It is not likely that it will be finished before this is released with the server. |
as soon as there is anything in the client available we will start testing asap - got that. |
|
integration test cool |
| */ | ||
| function checkMove($source, $destination) { | ||
| $sourceNode = $this->server->tree->getNodeForPath($source); | ||
| if ($sourceNode instanceof FutureFile) { | ||
| return; |
There was a problem hiding this comment.
Isn't that a bit to liberal? What if the file we are uploading to does not allow updates?
There was a problem hiding this comment.
This is a valid scenario we need to cover in the second part of the implementation which is know as 'Announcing the upload'
|
Lets do this! 👍 |
|
THX |
|
@DeepDiver1975 @dragotin from a qa pov, is it needed further testing in addition to integration ones? |
no - we need a client supporting this to setup all the smashboxing and so on |
|
@mcastroSG this is of your interest, perhaps you may help with the smashbox too |
hold your horses - as said we first need a client supporting the new chunking and use the new webdav endpoint |
|
Apparently ownbrander can be used for creating a client with a custom endpoint. Not sure if that can be used with smashbox in jenkins though. |
|
Ownbrander will not help. Support needs to be added to the client. |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
as specified in https://dragotin.wordpress.com/2015/06/22/owncloud-chunking-ng/
@dragotin here we go
@PVince81 @icewind1991 @schiesbn @rullzer @blizzz @nickvergessen
Please have a look - THX
Yes - more tests are to be written - on it