Skip to content
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.

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

Add max parallels to getcapabiltilies #13628

Closed
wants to merge 1 commit into from
Closed

Add max parallels to getcapabiltilies #13628

wants to merge 1 commit into from

Conversation

PVince81
Copy link
Contributor

If SQLite, limit parallel upload to 1

Still to be discussed @guruz (in case you change your mind)

If SQLite, limit parallel upload to 1
@ghost
Copy link

ghost commented Jan 23, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/7846/
🚀 Test PASSed. 🚀

@scrutinizer-notifier
Copy link

The inspection completed: 10 new issues

'files' => array(
'bigfilechunking' => true,
),
'files' => $caps
Copy link
Member

Choose a reason for hiding this comment

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

Add a , at EoL to not have to edit it for future adjustments?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the intendation is off here

@DeepDiver1975
Copy link
Member

Should we disallow syncing at all? Parallel upload can result in database locks but multiple clients from multiple users can trigger the same result.

For testing and development purpose I want to keep sqlite.

Same for low traffic usage - e.g. somebody who uses ownCloud and calendar and files platform for a pure sharing perspective. In such a case sqlite is really sufficient.

@PVince81
Copy link
Contributor Author

I'll only fix this PR once we agree on the approach.
I raised it in a rush for @guruz as a POC.

@karlitschek
Copy link
Contributor

Hmm. In which scenario is it harmful to try to do several WebDAV requests in parallel?
Doesn´t SQLite lock properly in case a lock is needed?

@guruz
Copy link
Contributor

guruz commented Jan 26, 2015

@karlitschek the busy wait handler was removed (if i understand) when switching from mdb2 to doctrine. #13615

@PVince81 ok.. and you'll also add a flag "using_legacy_unperformant_db" or so flag in case we ever want to display a message in the client? (as discussed with @LukasReschke )

ogoffart added a commit to owncloud/client that referenced this pull request Jan 26, 2015
If the server can't cope with parallel upload, it can advertise it
in the capabilities secion

Issue #2743
Need owncloud/core#13628
@LukasReschke
Copy link
Member

Another "SQLite does it all wrong"-issue that breaks our mobile clients: #13705

@karlitschek
Copy link
Contributor

I don´t understand this at all. It is possible to process several requests at the same time by using sqlite. We only need the proper locking working. This is best practices for web application for a long time and also worked in ownCloud like that. I don´t know what is currently broken in our code so that this is not working.
A capabilities call to "prevent" parallel requests is stupid. Sorry. We need to fix our sqlite handling here.

@PVince81
Copy link
Contributor Author

Can you guys confirm whether there was data loss involved in this situation ?
Or was this just an inconvenience for the user to see locking messages appearing too often ?

@karlitschek
Copy link
Contributor

A clarification about my previous comment. With the comment "stupid" I was referring to the approach that we work around a possible server bug that can lead to data lose by telling the client to not do parallel requests. If such a bug exists in the core that parallel requests can create data lose that we have to fix this bug and not tell the clients to not do parallel requests.
This wasn´t directed to a certain person or the work of a certain person.
Sorry for using bad words.

@DeepDiver1975
Copy link
Member

My pov on the current sqlite situation is described in here #13705 (comment)

@PVince81
Copy link
Contributor Author

@guruz can you test with #13752 with parallel upload ?
Is there a way I can test this too locally ?

@PVince81
Copy link
Contributor Author

I tested with #13752 and can confirm that it fixes the issue with parallel uploads. Would be good if @guruz can confirm this as well. Thanks.

@PVince81
Copy link
Contributor Author

Obsoleted by #13752

@PVince81 PVince81 closed this Jan 29, 2015
@PVince81 PVince81 deleted the maxparallels branch January 29, 2015 13:46
@guruz
Copy link
Contributor

guruz commented Jan 29, 2015

I guess you mean obsoleted by #13615 ?

@dragotin
Copy link
Contributor

I agree with @karlitschek, it does not make sense to limit the maximum numbers of parallel requests to the server for whatever reason or in whatever situation. While we can prevent the client from doing parallel requests we can not prevent users to use a script (for example) to get into the same situation. So that needs to be fixed, even if sqlite is used.

That is not easy to fix as @DeepDiver1975 points out. So we were looking for a solution for the upcoming releases (server 8 and client 1.8) and thus the idea of the capabilities sounds like a compromise for now.

ogoffart added a commit to owncloud/client that referenced this pull request Feb 5, 2015
If the server can't cope with parallel upload, it can advertise it
in the capabilities secion

Issue #2743
Need owncloud/core#13628
@lock lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants