-
-
Notifications
You must be signed in to change notification settings - Fork 860
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 support for batched local monitor processing #2609
Conversation
@JC-comp |
Conflicts fixed |
@JC-comp All directories should be created first (typically right away, not batched) so that they are 100% available online. This current PR, makes it appear that they are potentially made, but the Please look into this and potentially change your ordering / processing so that directories are processed first, before files are being uploaded. Evidence:
|
This behavior aligns with the original version's expectations, where some subfolders are processed last. There is no need to rearrange all folders to be processed first, as it will not cause any issues. Folders will still be guaranteed to be created before files are uploaded. Testing command:
|
@@ -6815,7 +6829,7 @@ class SyncEngine { | |||
if (!itemDB.selectByPath(oldPath, appConfig.defaultDriveId, oldItem)) { | |||
// The old path|item is not synced with the database, upload as a new file | |||
addLogEntry("Moved local item was not in-sync with local databse - uploading as new item"); | |||
uploadNewFile(newPath); | |||
scanLocalFilesystemPathForNewData(newPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why scan the entire filesystem, rather than pushing and using the uploadNewFile function directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scanLocalFilesystemPathForNewData
function can handle file uploads as well. In this new version, for cases like mkdir new_folder && mv new_folder new_loc
, new_folder will not trigger a redundant online creation (as local dir is gone). Consequently, it will not be in sync and will require a filesystem scan.
@JC-comp
One of the whiteboard items I have is how to integrate 'classify_as_big_delete' when using Is there a way for you to potentially integrate checking this config option against this total number of files|folders changed, so that the application stops the processing of the massive online delete ? Again this comes back to the processing order - directories should always be first, thus you can calculate the children of the path being deleted and negate that delete from being sent online if required. Please have a think about this on how this can be done with this PR. |
Given that each file/folder deletion triggers a separate inotify event, a naive implementation is to sum up these events and compare them with the classify_as_big_delete flag. |
At the moment, in v2.4.x here is no action - the delete is processed, but, v2.4.x and v2.5.x (before this PR) processes each delete action one at a time, making 'classify_as_big_delete flag' rather impossible. The only thought I had was - could something be done here? In Windows, a local delete of volumes of data, deletes online without any prevention. For the moment, unless some other idea can be found, I am OK to have the existing situation carried forward for |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This PR replaces the original one-by-one processing of the local file system monitor with batched processing for a single update, improving speed and consistency during periods of rapid changes.
Changes
Use case examples
$ touch 1 2 3 4 5
touch 1.txt && sleep 5 && echo "123" > 1.txt && mv 1.txt 2.txt