Optimize PUT - don't fetch and update checksum again, reunse the one from part file by mrow4a · Pull Request #27532 · owncloud/core

mrow4a · 2017-03-29T08:59:31Z

Back to the roots, but smarter https://github.com/owncloud/core/blob/stable8.2/lib/private/connector/sabre/file.php#L212

Unit tests

-3 SELECT, -1 UPDATE (!)

mention-bot · 2017-03-29T08:59:33Z

@mrow4a, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @PVince81 and @IljaN to be potential reviewers.

mrow4a · 2017-03-29T09:12:52Z

@DeepDiver1975 Any reasoning behind this ?

I cannot scratch my head for a use case, and I adjusted test to test the feature

mrow4a · 2017-03-30T09:28:05Z

Ready for review, needs more integration tests for checksums and PUT in general, smashbox it maybe?

jvillafanez · 2017-03-31T07:32:01Z

 			}

+			if ($view) {
+				$this->emitPostHooks($exists);


There is $this->fileView->touch below... This might change the behavior if the hooks are expected to run after that touch.

In previous versions https://github.com/owncloud/core/blob/stable8.2/lib/private/connector/sabre/file.php#L202 it was like in the link above, not sure what reasoning was behind moving it there @DeepDiver1975

jvillafanez · 2017-03-31T07:38:16Z

 	}

 	/**
+	 * Remove .path extension from a file path


"remove .path extension" 👀

Honestly, this is copy-paste from another function. We might think about creating separate class serving each layer in the code for partial files

core/lib/private/Files/Cache/Scanner.php

Line 461 in f10d105

public static function isPartialFile($file) {

core/lib/private/Encryption/Util.php

Line 252 in e4e9864

public function stripPartialFileExtension($path) {

Currently it is really spread over the code, which is bad. @DeepDiver1975 @jvillafanez What do you think?

I only wanted to change the wording: it should be ".part" not ".path" 😆

There should be only a function to handle this, maybe a class for all the string-based path manipulation, but this is currently out of scope.

jvillafanez · 2017-03-31T07:45:05Z

Has this been tested with big file chunking (old and / or new)? Are external storages included for the checksums?

mrow4a · 2017-03-31T08:59:19Z

I think new and old chunking code uses different functions, e.g. old chunking uses

core/apps/dav/lib/Connector/Sabre/File.php

Line 106 in 0496a82

return $this->createFileChunked($data);

External storages I did not test manualy, but it should not be a case, since it is lower layer (or at least should be and upper layer should not be aware of lower). Will tests it. @SergioBertolinSG is integration test for that, as @jvillafanez points out?

SergioBertolinSG · 2017-03-31T09:27:08Z

There are tests using local storage https://github.com/owncloud/core/blob/master/tests/integration/features/checksums.feature#L135

mrow4a · 2017-03-31T09:44:18Z

@SergioBertolinSG and external?

SergioBertolinSG · 2017-03-31T10:05:36Z

@SergioBertolinSG and external?

Only local set up as external storage.

mrow4a · 2017-03-31T10:09:37Z

@DeepDiver1975 Needs to review it carefully I think when he finds the time. @jvillafanez What about creating "PartialFilesUtils" class for the issues mentioned above?

jvillafanez · 2017-04-03T12:40:54Z

"PartialFilesUtils" suggests something only for dealing with part files, but I'd like to have something more generic. String-based path manipulations, which could include part file handling; without any FS access nor VFS access.

mrow4a · 2017-04-13T13:02:15Z

@DeepDiver1975 Is this scope of 10.0 to fix checksuming related performance?

mrow4a · 2017-05-23T21:14:38Z

Seems to me some kind of workaround for some purpose, probably for what I have erased to work

from my understanding this is necessary if the file was changed on disk which is detected by the scanner.
in this case the checksum has to be removed

I just went with debugger, first time the PUT gets there is with

core/apps/dav/lib/Connector/Sabre/File.php

Line 204 in 186595c

$storage->getUpdater()->update($internalPath);

, and

core/lib/private/Files/Cache/Scanner.php

Line 204 in 186595c

$data['fileid'] = $this->addToCache($file, $newData, $fileId);

$newData has following contents

Next time it gets there is in

core/apps/dav/lib/Connector/Sabre/File.php

Line 215 in 186595c

if ($this->fileView->touch($this->path, $request->server['HTTP_X_OC_MTIME'])) {

and it has following content

@DeepDiver1975 any thoughts?

I cannot think where cleaning of checksum might be usefull, first call goes through

core/lib/private/Files/Cache/Scanner.php

Line 200 in 186595c

$newData = $data;

, and second goes through

core/lib/private/Files/Cache/Scanner.php

Line 180 in 186595c

if (empty($cacheData['etag'])) {

Can you retest with an external storage ? Then change a file directly on external storage and run occ files:scan. In this case, the checksum in oc_filecache must disappear since the file changed and we didn't recompute it yet.

If this still works correctly then maybe there is yet another code paths that achieves the same result... strange

also beware, when testing uploads, always test these:

old endpoint, new file upload

old endpoint, upload + overwrite

old endpoint, new file chunked upload

old endpoint, chunked upload + overwrite

repeat above with new endpoint

please make sure these are covered by integration tests + checksums to make sure this PR doesn't break anything

mrow4a · 2017-05-23T21:15:19Z

It will "cache" checksum for the original file (for which .part file is created)

DeepDiver1975 · 2017-05-24T06:38:15Z

from my understanding this is necessary if the file was changed on disk which is detected by the scanner.
in this case the checksum has to be removed

PVince81 · 2017-05-30T11:52:33Z

There should be integration tests already about resetting the checksum when an external storage file change. Strange that #27532 (comment) did not trigger it.

mrow4a · 2017-06-21T10:59:25Z

For all Optimize PUT - this, #28021 and #28003 (comment) - I need proper smashbox and integration tests, and check if they verify what I do here. I will finish working on tests and verifying.

…rom part file

PVince81 · 2017-07-04T12:55:34Z

Rebased. master contains the integration tests I requested...

phil-davis · 2018-03-21T03:34:33Z

Note: this has not been backported to stable10
I happened to notice because of conflicts raised in backporting #30580 - that PR changed some indent of lines in Scanner.php e.g.

	// Reset the checksum if the data has changed
	$newData['checksum'] = '';

is taken out in master by this PR. But remains in stable10

Just noting here in case someone in future does a backport of this PR. You will get some conflict there.

mrow4a · 2018-05-06T08:35:02Z

@phil-davis @DeepDiver1975 @PVince81 I can see, in stable10 this is still not there. We still do there unnecessarily UPDATE - the checksumming information is not being reused from part file info, and requires to be additionally queried and updated.

Should I have a look at the conflict, or we kill this change?

mrow4a · 2018-05-06T08:45:53Z

Hmm, seems this to work would need to wait for #28003 due to removed conflic line - the mentioned PR removes the scan which cleared the checksum.

mrow4a · 2018-05-06T08:59:20Z

Hmm, it does not, since 523cd34 already solved it but detecting "real file change"

mrow4a · 2018-05-06T09:00:11Z

@PVince81 should I create backport PR and check everything again? Probably needs #31082 this first to resolve the conflict

lock · 2019-07-31T08:02:34Z

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.

mrow4a added the 2 - Developing label Mar 29, 2017

mrow4a added this to the 10.0 milestone Mar 29, 2017

mrow4a requested review from DeepDiver1975, IljaN and jvillafanez March 30, 2017 00:22

mrow4a added 3 - To Review and removed 2 - Developing labels Mar 30, 2017

jvillafanez reviewed Mar 31, 2017

View reviewed changes

PVince81 modified the milestones: 10.0.1, 10.0 Apr 26, 2017

DeepDiver1975 modified the milestones: 10.0.1, 10.1 May 17, 2017

mrow4a force-pushed the optimize_put_4 branch from 4f3bddb to 0c8fe79 Compare May 23, 2017 21:10

mrow4a commented May 23, 2017

View reviewed changes

mrow4a force-pushed the optimize_put_4 branch from 0c8fe79 to 186595c Compare May 23, 2017 21:16

DeepDiver1975 requested changes May 24, 2017

View reviewed changes

mrow4a assigned PVince81 May 24, 2017

mrow4a mentioned this pull request May 24, 2017

Optimize PUT - with custom mtime, use storage instead of view #28003

Merged

3 tasks

Optimize PUT - dont fetch and update checksum again, reunse the one f…

dd9dfdc

…rom part file

PVince81 force-pushed the optimize_put_4 branch from 186595c to dd9dfdc Compare July 4, 2017 12:55

PVince81 added 4 - To release and removed 3 - To Review labels Jul 5, 2017

PVince81 merged commit be92925 into master Jul 5, 2017

PVince81 deleted the optimize_put_4 branch July 5, 2017 14:56

DeepDiver1975 modified the milestones: 10.1, development Oct 10, 2017

phil-davis mentioned this pull request Mar 21, 2018

Test unlock on exception during scanning #30580

Merged

9 tasks

mrow4a changed the title ~~Optimize PUT - dont fetch and update checksum again, reunse the one f…~~ Optimize PUT - don't fetch and update checksum again, reunse the one from part file May 6, 2018

mrow4a added 3 - To Review and removed 4 - To release labels May 6, 2018

mrow4a mentioned this pull request Jun 13, 2018

[stable10] Optimize PUT - don't fetch and update checksum again, reuse the one from part file #31768

Merged

lock Bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Uh oh!

Conversation

mrow4a commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Mar 29, 2017

Uh oh!

mrow4a commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrow4a commented Mar 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrow4a Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jvillafanez commented Mar 31, 2017

Uh oh!

mrow4a commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergioBertolinSG commented Mar 31, 2017

Uh oh!

mrow4a commented Mar 31, 2017

Uh oh!

SergioBertolinSG commented Mar 31, 2017

Uh oh!

mrow4a commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jvillafanez commented Apr 3, 2017

Uh oh!

mrow4a commented Apr 13, 2017 • 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!

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!

PVince81 commented May 30, 2017

Uh oh!

mrow4a commented Jun 21, 2017

Uh oh!

PVince81 commented Jul 4, 2017

Uh oh!

phil-davis commented Mar 21, 2018

Uh oh!

mrow4a commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrow4a commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrow4a commented May 6, 2018

Uh oh!

mrow4a commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

mrow4a commented Mar 29, 2017 •

edited

Loading

mrow4a commented Mar 29, 2017 •

edited

Loading

mrow4a Mar 31, 2017 •

edited

Loading

mrow4a commented Mar 31, 2017 •

edited

Loading

mrow4a commented Mar 31, 2017 •

edited

Loading

mrow4a commented Apr 13, 2017 •

edited

Loading

mrow4a commented May 6, 2018 •

edited

Loading

mrow4a commented May 6, 2018 •

edited

Loading

mrow4a commented May 6, 2018 •

edited

Loading