Classify chunked items correctly. Issue #5850 #5852

mrow4a · 2017-06-20T18:32:36Z

This should solve #5850.

Please note: we cannot set chunk size of PUT V1 to _chunkSize which is dynamic because PUT V1 will not adjust dynamic chunk size. If we want it, we need to make code adjusting dynamic chunk size both for V1 and NG.

mrow4a · 2017-06-20T18:35:50Z

mrow4a · 2017-06-20T18:37:14Z

src/libsync/owncloudpropagator.cpp

@@ -379,7 +379,8 @@ PropagateItemJob *OwncloudPropagator::createJob(const SyncFileItemPtr &item)
            return job;
        } else {
            PropagateUploadFileCommon *job = 0;
-            if (item->_size > _chunkSize && account()->capabilities().chunkingNg()) {


Because of that, after dynamic chunk has been readjusted to 100MB, this file has been classified as V1, being 50MB in size. With _initialChunkSize being 10MB, it is classified as to be chunked and having high dynamic chunk, being send in one shot.

yes, that's correct. Thanks for spotting and fixing this.

ogoffart

Thanks for fixing this. Please just fix or remove the missleading comment before pushing.

ogoffart · 2017-06-21T06:39:34Z

src/libsync/propagateuploadng.cpp

@@ -400,6 +400,7 @@ void PropagateUploadFileNG::slotPutFinished()
        // the chunk sizes a bit.
        quint64 targetSize = (propagator()->_chunkSize + predictedGoodSize) / 2;

+        // Adjust chunk size for the item being classified to be chunked.


this comment is wrong. This has nothing to do with the classification, quite the contrary. It is the actual size that will be used by the chunking-ng algorithm

Hmm, maybe I need to rewrite this. The intiention was to say that this is the dynamic chunk size of the item, which has been classified to be chunked.

ogoffart · 2017-06-21T06:41:51Z

src/libsync/owncloudpropagator.cpp

@@ -379,7 +379,8 @@ PropagateItemJob *OwncloudPropagator::createJob(const SyncFileItemPtr &item)
            return job;
        } else {
            PropagateUploadFileCommon *job = 0;
-            if (item->_size > _chunkSize && account()->capabilities().chunkingNg()) {


yes, that's correct. Thanks for spotting and fixing this.

guruz · 2017-06-21T08:00:04Z

src/libsync/propagateuploadv1.cpp

@@ -79,6 +79,7 @@ void PropagateUploadFileV1::startNextChunk()
    qint64 chunkStart = 0;
    qint64 currentChunkSize = fileSize;
    bool isFinalChunk = false;
+


If you anyway change the commit, remove this one too :)

mrow4a · 2017-06-21T11:06:09Z

Done

ogoffart

👍

ckamm

With this change we'll start using chunked upload via PropagateUploadFileNG for files that will only have a single chunk. I assume that was tested and is safe.

ckamm · 2017-06-22T11:43:57Z

src/libsync/discoveryphase.h

@@ -53,9 +53,10 @@ struct SyncOptions
    /** If a confirmation should be asked for external storages */
    bool _confirmExternalStorage;

-    /** The initial un-adjusted chunk size in bytes for chunked uploads
+    /** The initial un-adjusted chunk size in bytes for chunked uploads, both
+     * for old and new chunking algorithm, which classifies the item to be chunked


When you say "which classifies the item to be chunked" (also below) that's confusing because it's ambiguous. "which is used to decide whether an item should be chunked" would be clearer.

mrow4a · 2017-06-27T09:27:19Z

@guruz why the jenkins test is not passing (it is also not for master), can we merge this PR?

ogoffart · 2017-07-03T11:18:42Z

@mrow4a the jenkins error seems to be unrelated to this change:

6: QWARN  : TestSyncJournalDB::testFileRecord() sync.database.sql: Sqlite exec statement error: 5 "database is locked" in "INSERT OR IGNORE INTO checksumtype (name) VALUES (?1)"
6: QWARN  : TestSyncJournalDB::testFileRecord() sync.database.sql: Sqlite exec statement error: 5 "database is locked" in "INSERT OR REPLACE INTO metadata (phash, pathlen, path, inode, uid, gid, mode, modtime, type, md5, fileid, remotePerm, filesize, ignoredChildrenRemote, contentChecksum, contentChecksumTypeId) VALUES (?1 , ?2, ?3 , ?4 , ?5 , ?6 , ?7,  ?8 , ?9 , ?10, ?11, ?12, ?13, ?14, ?15, ?16);"
6: FAIL!  : TestSyncJournalDB::testFileRecord() 'storedRecord == record' returned FALSE. ()
6:    Loc: [/var/lib/jenkins/workspace/cloud-client_client_PR-5852-5C3JT3EY7EHDKWSDVRI76HKJJMTNLBZ76KGHLTTGG25CKR2ASZYA/test/testsyncjournaldb.cpp(78)]

I'd say we can merge this change. yes

mrow4a requested review from ckamm, jturcotte, ogoffart and guruz June 20, 2017 18:32

mrow4a commented Jun 20, 2017

View reviewed changes

mrow4a mentioned this pull request Jun 20, 2017

chunked uploads: Don't mix upload V1 and NG #5850

Closed

ogoffart reviewed Jun 21, 2017

View reviewed changes

guruz reviewed Jun 21, 2017

View reviewed changes

guruz added this to the 2.4.0 milestone Jun 21, 2017

mrow4a force-pushed the fix_chunking_ng branch from d7e3dbd to 32d7c56 Compare June 21, 2017 11:04

Classify chunked items correctly. Issue #5850

a43cf2a

mrow4a force-pushed the fix_chunking_ng branch from 32d7c56 to a43cf2a Compare June 21, 2017 11:05

ogoffart approved these changes Jun 21, 2017

View reviewed changes

ckamm reviewed Jun 22, 2017

View reviewed changes

ogoffart mentioned this pull request Jun 23, 2017

Client uses old chunking protocol with new DAV endpoint "Precondition failed" "An If-Match header was specified and the resource did not exist" #5855

Closed

guruz merged commit d1e0009 into master Jul 3, 2017

guruz deleted the fix_chunking_ng branch July 3, 2017 12:41

michaelstingl mentioned this pull request Apr 13, 2020

Decide on HTTP chunked-data transfer protocol cs3org/reva#290

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Classify chunked items correctly. Issue #5850 #5852

Classify chunked items correctly. Issue #5850 #5852

mrow4a commented Jun 20, 2017

mrow4a commented Jun 20, 2017

mrow4a Jun 20, 2017

ogoffart Jun 21, 2017

ogoffart left a comment

ogoffart Jun 21, 2017

mrow4a Jun 21, 2017

ogoffart Jun 21, 2017

guruz Jun 21, 2017

mrow4a commented Jun 21, 2017

ogoffart left a comment

ckamm left a comment

ckamm Jun 22, 2017

mrow4a commented Jun 27, 2017 •

edited

Loading

ogoffart commented Jul 3, 2017

Classify chunked items correctly. Issue #5850 #5852

Classify chunked items correctly. Issue #5850 #5852

Conversation

mrow4a commented Jun 20, 2017

mrow4a commented Jun 20, 2017

mrow4a Jun 20, 2017

Choose a reason for hiding this comment

ogoffart Jun 21, 2017

Choose a reason for hiding this comment

ogoffart left a comment

Choose a reason for hiding this comment

ogoffart Jun 21, 2017

Choose a reason for hiding this comment

mrow4a Jun 21, 2017

Choose a reason for hiding this comment

ogoffart Jun 21, 2017

Choose a reason for hiding this comment

guruz Jun 21, 2017

Choose a reason for hiding this comment

mrow4a commented Jun 21, 2017

ogoffart left a comment

Choose a reason for hiding this comment

ckamm left a comment

Choose a reason for hiding this comment

ckamm Jun 22, 2017

Choose a reason for hiding this comment

mrow4a commented Jun 27, 2017 • edited Loading

ogoffart commented Jul 3, 2017

mrow4a commented Jun 27, 2017 •

edited

Loading