nextcloud / integration_dropbox

🧊 Dropbox integration into Nextcloud
GNU Affero General Public License v3.0
24 stars 10 forks source link

Refactor #10

Closed vitormattos closed 3 years ago

vitormattos commented 3 years ago

Hi @eneiluj , I'm reviewing your app's code. I want to improve performance and parallelize the download of Dropbox files.

Is it okay for you to send separate pull requests to make it easier to review the code?

In this pull request I removed an unused property and added the service property to improve coding in an IDE.

julien-nc commented 3 years ago

Hi @vitormattos,

As long as the changes don't mix multiple different things/topics, you can make big PRs, i don't mind. Do you want to add stuff to this PR or can I merge it right now?

julien-nc commented 3 years ago

And by the way, as Dropbox most likely sends faster than everybody can receive, I don't feel it's necessary to parallelize file download as you're still going to get stuck at your connection max speed.

vitormattos commented 3 years ago

For now nothing to add to it.

Because the app is without unit tests, I'm doing manual tests. I think it's safer to go for smaller parts so as not to get too complex when doing the review.

vitormattos commented 3 years ago

I need to transfer more than 600 GB from Dropbox to the Nextcloud and for some reason the job is dying without logs after about 60 GB transferred.

Importing files has other processes besides downloading, for example IO. When the import is done in a queue and for many files, the time increases.

The goal is to parallel a poll of up to 5 requests so that, while a download is being made, a response promise from another download is being processed.

vitormattos commented 3 years ago

Just in time, I can put this pull request as a draft and add everything to it. Finally, you do one last review. Do you prefer it this way?

Now that I recognize you, hi @eneiluj !!!

vitormattos commented 3 years ago

Hi @eneiluj , I made some refactors and I'm testing with the import of 600Gb, it is without problems. I suggest you do the code review by the sequence of commits.

julien-nc commented 3 years ago

You forgot to import OCP\Files\ForbiddenException :wink:.

vitormattos commented 3 years ago

@eneiluj done.