Chunking ng by ogoffart · Pull Request #5102 · owncloud/client

ogoffart · 2016-08-03T15:48:24Z

No description provided.

mention-bot · 2016-08-03T15:48:26Z

@ogoffart, thanks for your PR! By analyzing the annotation information on this pull request, we identified @ckamm, @jturcotte and @dragotin to be potential reviewers

guruz · 2016-08-04T09:35:59Z

    void setSharedThis(AccountPtr sharedThis);
    AccountPtr sharedFromThis();

+    /// The user that can be used in dav url


please comment how this relates to the shibboleth user or the user name entered in the wizard

I don't know exactly. the answer from @DeepDiver1975 on #engineering (17:26) was not enough for me to make a better comment.

…implemented

Current limitations of this WiP - No resuming implemented yet - No parallel chunks - Hackish way to get the webdav paths

dragotin · 2016-08-31T08:48:24Z

                return job;
            } else {
-                auto job = new PropagateUploadFile(this, item);
+                static const bool isNg  = !qgetenv("OWNCLOUD_CHUNK_NG").isEmpty(); // FIXME! use server version


I think it should be rather decided by a server capability. @DeepDiver1975 does the new chunking already add something to the capabilities?

Yes of course, this is debug only right now.

ogoffart · 2016-10-31T10:30:39Z

Ready to be reviewed

guruz · 2016-10-31T11:28:25Z

I think this code is not defensive enough, see my comment with regards to dynamic chunk sizes etc.

Does the server always check transmission checksums by now?

Also: Where are the tests for tx.pl and smashbox?

PVince81 · 2016-11-02T10:10:46Z

Does the server always check transmission checksums by now?

This is not implemented yet.

ogoffart · 2016-11-04T15:32:53Z

Latests commits added tests, and remove the extra chunks before starting an upload

ckamm · 2016-11-07T10:28:05Z


    AccountPtr sharedFromThis();

+    /// The user that can be used in dav url


Can you add a comment explaining in what situations that would be different from credentials.user()?

I don't know. I was just told it could be different and that i had to do an API call even when using the HTTPCrendentials.

@DeepDiver1975 Can you clarify?

ckamm · 2016-11-07T10:40:14Z

+ *    slotComputeTransmissionChecksum()
+ *         |
+ *         v
+ *    slotStartUpload()  -> doStartUpload()


Documentation! Yay!

ckamm · 2016-11-07T10:43:04Z

+    // (Only used from slotPropfindIterate/slotPropfindFinished because the LsColJob use signals to report data.)
+    QMap<int, quint64> _serverChunks;
+
+    quint64 chunkSize() const { return _propagator->chunkSize(); }


Could move to Common?

Well, the idea would be that we could do dynamic chunking, so they would be different.

ckamm · 2016-11-07T10:47:00Z

+
+/*
+  State machine:
+


Again: very very helpful

ckamm · 2016-11-07T10:49:34Z

+     * If chunk == -1, returns the URL of the parent folder containing the chunks
     */
-    void setDeleteExisting(bool enabled);
+    QUrl chunkUrl(int chunk = -1);


minor: I'd prefer if there was a chunkFolderUrl() and a chunkUrl(int)

yeah, that's true. But it's internal function anywaéy

ckamm · 2016-11-07T10:56:17Z

+    _propagator->_journal->setUploadInfo(_item->_file, pi);
+    _propagator->_journal->commit("Upload info");
+    QMap<QByteArray, QByteArray> headers;
+    headers["OC-Total-Length"] = QByteArray::number(_item->_size);


When we're resuming - is there a way for us to know that the directory we're resuming with has the same OC-Total-Length?

Yes, because the _transferId would be different (the size is xor'ed with it)

ckamm · 2016-11-07T10:57:34Z

+
+    quint64 fileSize = _item->_size;
+
+    quint64 currentChunkSize = qMin(chunkSize(), fileSize - _sent);


As pointed out above, in bad resumes fileSize might be smaller than _sent.

No, because the file need to be identical.

ckamm · 2016-11-07T10:57:57Z

+
+    quint64 currentChunkSize = qMin(chunkSize(), fileSize - _sent);
+
+    if (currentChunkSize <= 0) {


<= 0 is a bit misleading, since this is a quint64

ckamm · 2016-11-07T10:59:16Z

+        Q_ASSERT(_jobs.isEmpty()); // There should be no running job anymore
+        _finished = true;
+        // Finish with a MOVE
+        // QString destination = _propagator->_remoteDir + _item->_file;  // FIXME: _remoteDir currently is still using the old webdav path


outdated comment

ckamm · 2016-11-07T11:02:50Z

+        return;
+    }
+
+    bool finished = _sent >= _item->_size;


Shouldn't sent > _item->_size be a fatal error? How would that happen?

You are right, this can't happen, i'll replace with a ==

ogoffart · 2016-11-07T11:45:58Z

@guruz the client send the checksum header (for the whole file) in the MOVE operation.
I think it would be overkill to do checksum check on every chunk.

guruz · 2016-11-08T09:51:05Z

I'm working on a server patch that does whole file (not chunk) checksum verifying for chunking ng (only).
Then the client code could check if Capabilities::supportedChecksumTypes returns something and only then enable chunking ng.

This is for owncloud/client#5102 (comment)

DeepDiver1975 · 2016-11-11T09:15:16Z

+
+using namespace OCC;
+
+class TestChunkingNG : public QObject


I see the tmp folder being polluted with folders named ChunkingNG-* - is there some cleanup missing?

root@ci:/tmp/ChunkingNgTest-AQJojC/A# ls -lh total 287M -rw-r--r-- 1 jenkins jenkins 287M Nov 11 02:34 a0 -rw-r--r-- 1 jenkins jenkins 4 Nov 4 02:35 a1 -rw-r--r-- 1 jenkins jenkins 4 Nov 4 02:35 a2

The cleanup is there if the test does not crash. But it seems that the test is crashing. need to investigate why.

I see - thanks!

As the file can be some hunreds of megabytes, allocating such big arrays may cause problems. Also make the timeout a bit bigger so the test can rununder valgrind.

ogoffart · 2016-11-15T14:31:12Z

Checksuming is a server issue and is orthogonal to this problem

guruz · 2016-11-15T14:58:26Z

@SamuAlfageme This needs to be tested with a server 9.2 (e.g. a quite current master branch of the server), previous servers don't have this chunking ng support.

SamuAlfageme · 2016-11-15T15:02:54Z

@guruz noted, will do 😉

guruz reviewed Aug 4, 2016
View reviewed changes

ogoffart added 5 commits August 31, 2016 10:28

Upload: refactor the upload in two classes so the new chuning can be …

4f3f642

…implemented

WIP: new chunking algorithm

a155810

Current limitations of this WiP - No resuming implemented yet - No parallel chunks - Hackish way to get the webdav paths

Chunking-Ng: Resume

fad387b

ChunkingNg: enable depending on an environment variable

79abb8b

Chunking-NG: Qt4 compile

818b585

ogoffart force-pushed the chunking-ng branch from 6aa4c33 to 818b585 Compare August 31, 2016 08:29

dragotin reviewed Aug 31, 2016
View reviewed changes

Fix compile after merge

456d827

ogoffart assigned guruz and ckamm and unassigned ogoffart Oct 31, 2016

guruz added the DO NOT MERGE YET label Oct 31, 2016

ChunkingNG: Add Test

c8014a0

ogoffart removed the DO NOT MERGE YET label Nov 4, 2016

ChunkingNG: remove stale files when resuming

15f2b91

ogoffart force-pushed the chunking-ng branch from 796bc4c to 15f2b91 Compare November 4, 2016 15:43

ckamm reviewed Nov 7, 2016

View reviewed changes

guruz added a commit to owncloud/core that referenced this pull request Nov 8, 2016

Chunking NG: SHA1 checksumming of whole file #11811

b15a74f

This is for owncloud/client#5102 (comment)

guruz mentioned this pull request Nov 8, 2016

Chunking NG: SHA1 checksumming of whole file #11811 owncloud/core#26582

Closed

DeepDiver1975 reviewed Nov 11, 2016

View reviewed changes

Chunking-NG: code cleanup after review

5377d1e

guruz added the DO NOT MERGE YET label Nov 15, 2016

SyncEngineTestUtils: Do don't allocate a buffer for the whole file

92027e8

As the file can be some hunreds of megabytes, allocating such big arrays may cause problems. Also make the timeout a bit bigger so the test can rununder valgrind.

ogoffart removed the DO NOT MERGE YET label Nov 15, 2016

guruz merged commit 52552a4 into master Nov 15, 2016

guruz deleted the chunking-ng branch November 15, 2016 14:57

guruz mentioned this pull request Nov 15, 2016

Implement new Chunking Algorithm #4019

Closed


		AccountPtr sharedFromThis();

		/// The user that can be used in dav url


		quint64 fileSize = _item->_size;

		quint64 currentChunkSize = qMin(chunkSize(), fileSize - _sent);


		quint64 currentChunkSize = qMin(chunkSize(), fileSize - _sent);

		if (currentChunkSize <= 0) {

Uh oh!

Conversation

ogoffart commented Aug 3, 2016

Uh oh!

mention-bot commented Aug 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogoffart commented Oct 31, 2016

Uh oh!

guruz commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Nov 2, 2016

Uh oh!

ogoffart commented Nov 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogoffart commented Nov 7, 2016

Uh oh!

guruz commented Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogoffart commented Nov 15, 2016

Uh oh!

guruz commented Nov 15, 2016

Uh oh!

SamuAlfageme commented Nov 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

guruz commented Oct 31, 2016 •

edited

Loading

guruz commented Nov 8, 2016 •

edited

Loading