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

Improve confinement #8710

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Improve confinement #8710

wants to merge 3 commits into from

Conversation

vxgmichel
Copy link
Contributor

@vxgmichel vxgmichel commented Oct 12, 2024

More improvements over #8682

Fix #7885

Namely:

  1. Swap the logic in merge_local_folder_manifest: the original implementation performed the merging of the remote and local children using confined version of the correspond manifests (i.e applying the local confinement context on the remote). But in order to that, both manifests need to use the same prevent_sync_pattern for consistency, so there had to be an extra step of applying the new prevent sync pattern on the local manifest, even though it hasn't changed in basically all cases. This made the implementation of merge_local_folder_manifest more confusing than it needs to be: instead, the merging of the children can be made on the unconfined version of the corresponding manifests. Now the manifests have no reasons to be outdated, which remove the need for the extra conversion entirely.
    To sum it up, the new implementation does this:

         local manifest
             |
             [remove confinement]
             |
             v
         unconfined local manifest
             |
             [merge] <- using unconfined remote manifest 
             |
             v
         new unconfined local manifest
             |
             [apply confinement] <- using local manifest 
             |
             v
         new local manifest
    

    Note that this process is similar to applying a new pattern (or re-applying the existing one) if the remote manifest includes no changes.
    Note that this doesn't affect the overall behavior of merge_local_folder_manifest aside for some specific cases here and there.
    Note that this fixes merge_local_folder_manifest should expect local to respect the prevent_sync_pattern #7885

  2. A change to apply_confinement allows clients to keep their synchronized files locally when the happen to match a new prevent_sync_pattern. In the previous implementation, the file would have simply been removed. This made sense because in the general case, we want to avoid referencing an id locally (and possibly update its corresponding content) when it's also remotely confined. But in the special case where the entry is both confined locally and remotely, there is no problem keeping the id both locally and remotely, as long as the corresponding confined entry is not synchronized. Handling this specific case then allows for client to keep their temporary files when their names starts matching the prevent sync pattern, which is likely a desirable behavior.
    However, it depends on the following issue being addressed, which is not the case at the moment: Manage confined entries in synchronization monitor #8198
    The new policy for restoring local context is explained here: https://github.com/Scille/parsec-cloud/pull/8710/files#diff-946f0b5484866471f0fca02ee282c7bf63992b11af4419da76c7fe0712ce2d91R395-R404

This PR also asks the following question:

    // The following case was present in the original v2 code.
    // TODO: investigate why it's not present anymore and check whether
    // it is still relevant.
    // ```python
    // # All the local changes have been successfully uploaded
    // if local_manifest.match_remote(remote_manifest):
    //     return local_from_remote
    // ```

Comment on lines +312 to +319
// The following case was present in the original v2 code.
// TODO: investigate why it's not present anymore and check whether
// it is still relevant.
// ```python
// # All the local changes have been successfully uploaded
// if local_manifest.match_remote(remote_manifest):
// return local_from_remote
// ```
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it something you will investigate during this PR or at a later time ?

If it's the later, could you create an issue and add its link in the TODO command ?

if existing_local_manifest
.local_confinement_points
.contains(entry_id)
{
// Insert the locally confined entry in the new manifest
existing_local_confined_entries.insert(name.clone(), Some(*entry_id));

// Perform a rename if the entry id is already present in the new manifest
if let Some(&previous_name) = new_entry_ids.get(entry_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check if previous != name ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

merge_local_folder_manifest should expect local to respect the prevent_sync_pattern
2 participants