Skip to content

Optimize PUT - Dont fetch info of not existing file #28021

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.

Sign up for GitHub

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

Jump to bottom
Merged
PVince81 merged 2 commits into from
Aug 14, 2017
Merged

Optimize PUT - Dont fetch info of not existing file #28021

PVince81 merged 2 commits into from
Aug 14, 2017

Conversation

@mrow4a
Copy link
Contributor

@mrow4a mrow4a commented May 26, 2017 •
edited
Loading

"in case the file already exists/overwriting" ? In createFile() function which from composer is used only for new files, and there is specific one called updateFile() ?

-1 query plus Locking/Releasing because each getFileInfo needs to do that
selection_295

@mrow4a mrow4a requested review from DeepDiver1975 and PVince81 May 26, 2017 13:05
@PVince81
Copy link
Contributor

PVince81 commented May 30, 2017

Did you test ? Is updateFile() called for overwrites and not createfile() ? It is likely that you are right.

From what I remember, an overwrite still goes to File::put() though.

@PVince81
Copy link
Contributor

PVince81 commented May 30, 2017 •
edited by mrow4a
Loading

make sure to test the following:

  • old endpoint overwrite upload
  • old endpoint chunked upload+overwrite
  • new endpoint overwrite upload
  • new endpoint chunked upload-overwrite

there is a slight chance that the chunked mode will go through createFile() even for overwrites... (yes, messy stuff here...)

@PVince81
Copy link
Contributor

PVince81 commented Jun 21, 2017

@mrow4a any update on my comments ?

@mrow4a mrow4a mentioned this pull request Jun 21, 2017
Merged
1 task
@PVince81
Copy link
Contributor

PVince81 commented Jul 4, 2017

@mrow4a please rebase. On master I've added chunking + overwrite tests so the integration tests will take care of verifying your change.

If tests pass, then 👍

@PVince81
Copy link
Contributor

PVince81 commented Jul 4, 2017

let me rebase that for you

@PVince81 PVince81 force-pushed the optimize_put_6 branch 2 times, most recently from 7c13305 to 29b81b4 Compare July 5, 2017 12:02
@PVince81
Copy link
Contributor

PVince81 commented Jul 5, 2017 

Resuming build at Tue Jul 04 13:24:01 CEST 2017 after Jenkins restart
Ready to run at Tue Jul 04 13:24:02 CEST 2017
13:24:02 Timeout set to expire in 1 hr 29 min

Rebased ⛏

@PVince81
Copy link
Contributor

PVince81 commented Jul 5, 2017

some apparently legit failures:

16:35:57 --- Failed scenarios:
16:35:57 
16:35:57     /var/lib/jenkins/workspace/owncloud-core_core_PR-28021-LBIG7E7PMTL3SCH6VDBSM2DPVZILEZZV2SKQOI7W3QQLEFOBH4CQ/tests/integration/features/quota.feature:24
16:35:57     /var/lib/jenkins/workspace/owncloud-core_core_PR-28021-LBIG7E7PMTL3SCH6VDBSM2DPVZILEZZV2SKQOI7W3QQLEFOBH4CQ/tests/integration/features/quota.feature:74
16:35:57     /var/lib/jenkins/workspace/owncloud-core_core_PR-28021-LBIG7E7PMTL3SCH6VDBSM2DPVZILEZZV2SKQOI7W3QQLEFOBH4CQ/tests/integration/features/quota.feature:107
16:35:57     /var/lib/jenkins/workspace/owncloud-core_core_PR-28021-LBIG7E7PMTL3SCH6VDBSM2DPVZILEZZV2SKQOI7W3QQLEFOBH4CQ/tests/integration/features/webdav-related-new-endpoint.feature:482

@mrow4a please have a look

@mrow4a mrow4a force-pushed the optimize_put_6 branch 2 times, most recently from 6fc99b9 to 9090ba3 Compare August 3, 2017 08:43
@PVince81
Copy link
Contributor

PVince81 commented Aug 7, 2017 

01:48:30 
01:48:30 --- Failed scenarios:
01:48:30 
01:48:30     /var/lib/jenkins/workspace/owncloud-core_core_PR-28021-LBIG7E7PMTL3SCH6VDBSM2DPVZILEZZV2SKQOI7W3QQLEFOBH4CQ/tests/integration/features/quota.feature:24
01:48:30     /var/lib/jenkins/workspace/owncloud-core_core_PR-28021-LBIG7E7PMTL3SCH6VDBSM2DPVZILEZZV2SKQOI7W3QQLEFOBH4CQ/tests/integration/features/quota.feature:74
01:48:30     /var/lib/jenkins/workspace/owncloud-core_core_PR-28021-LBIG7E7PMTL3SCH6VDBSM2DPVZILEZZV2SKQOI7W3QQLEFOBH4CQ/tests/integration/features/quota.feature:107
01:48:30     /var/lib/jenkins/workspace/owncloud-core_core_PR-28021-LBIG7E7PMTL3SCH6VDBSM2DPVZILEZZV2SKQOI7W3QQLEFOBH4CQ/tests/integration/features/webdav-related-new-endpoint.feature:519
01:48:30

@PVince81
Copy link
Contributor

PVince81 commented Aug 10, 2017 


  | Test Result (2 failures  / -2)OCA\DAV\Tests\unit\Connector\Sabre\RequestTest\UploadTest.testBasicUploadOCA\DAV\Tests\unit\Connector\Sabre\RequestTest\PartFileInRootUploadTest.testBasicUpload
-- | --

@PVince81 PVince81 added this to the development milestone Aug 10, 2017
@mrow4a
Copy link
Contributor Author

mrow4a commented Aug 10, 2017

Unit tests are not adjusted because they don't pass PATH_INFO for $SERVER

@mrow4a
Copy link
Contributor Author

mrow4a commented Aug 10, 2017

@jvillafanez @PVince81 @DeepDiver1975 review here?

PVince81
PVince81 reviewed Aug 10, 2017
View reviewed changes
apps/dav/lib/Upload/FutureFile.php
$davUploadsTarget = '/dav/uploads';

// Check if pathinfo starts with dav uploads target and basename is future file basename
if (isset($_SERVER['PATH_INFO'])
Copy link
Contributor

@PVince81 PVince81 Aug 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any way to do this without global variables + static functions ? (pretty sure @DeepDiver1975 would not be happy either)

I do see that it is likely rather tricky to inject all this down to the "File" class.
@DeepDiver1975 should we pass on this one ?

Copy link
Contributor Author

@mrow4a mrow4a Aug 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, it was just the suggestion, but wanted to create a specific class which might hold all these values.

Copy link
Contributor Author

@mrow4a mrow4a Aug 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the global, not sure if request holds this value alone.

PVince81
PVince81 approved these changes Aug 14, 2017
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 fine by me

@PVince81 PVince81 merged commit 3b0f072 into master Aug 14, 2017
@PVince81 PVince81 deleted the optimize_put_6 branch August 14, 2017 14:28
@PVince81
Copy link
Contributor

PVince81 commented Aug 14, 2017

@mrow4a please backport to stable10

@mrow4a mrow4a mentioned this pull request Aug 16, 2017
Merged
@lock
Copy link

lock bot commented Aug 2, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

1 more reviewer

@PVince81 PVince81 PVince81 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mrow4a mrow4a

Labels

3 - To Review

Projects

None yet

Milestone

10.0.3

Development

Successfully merging this pull request may close these issues.

3 participants

@mrow4a @PVince81