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

HDDS-8435. [Snapshot] Handle Directory renames for FSO Buckets. #4607

Merged
merged 4 commits into from
May 2, 2023

Conversation

aswinshakil
Copy link
Member

What changes were proposed in this pull request?

When directories are renamed, this information is not tracked. It is required by theSnapshotDeletingService to check the previous snapshot's directoryTable and check if the renamed key is present.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-8435

How was this patch tested?

Existing tests.

@aswinshakil aswinshakil added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Apr 21, 2023
@aswinshakil aswinshakil self-assigned this Apr 21, 2023
@jojochuang
Copy link
Contributor

HI i haven't reviewed the PR because i don't know much about the snapshot design.
But I know for a fact directory rename could be problematic for FSO. For example, what happens if a file is being written under a directory that is renamed? It will file to commit. Has this been considered?

@aswinshakil
Copy link
Member Author

The snapshotRenamedTable only tracks the tableKey of the directory that was renamed and it is only used by SnapshotDeletingService. When a directory is renamed after a snapshot for a bucket is taken. The SnapshotDeletingService looks into the directoryTable of the previous snapshot, but when the directory is renamed the tableKey will be changed. The snapshotRenamedTable only acts as reference to check the renamed directories in between snapshot.

For example, what happens if a file is being written under a directory that is renamed? It will file to commit. Has this been considered?

This shouldn't be an issue, Everything would go through the original flow.

Copy link
Contributor

@smengcl smengcl left a comment

Choose a reason for hiding this comment

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

lgtm overall

@aswinshakil
Copy link
Member Author

Thanks @smengcl for the review. I have updated the PR, please take a look at it.

Copy link
Contributor

@smengcl smengcl left a comment

Choose a reason for hiding this comment

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

lgtm so far.

@GeorgeJahad mentioned he would want to scrutinize the test cases.

Though we have the renameDir addition here. We might want to have more test cases around mixtures of key deletes, renames and snapshot deletes.

e.g.

  1. create keys and dirs
  2. create snapshot 1
  3. rename keys and dirs
  4. verify key and rename table states
  5. create snapshot 2
  6. rename some of the keys and dirs, delete some of the keys and dirs
  7. verify table states
  8. create snapshot 3
  9. again rename some of the keys and dirs, delete some of the keys and dirs
  10. create snapshot 4
  11. verify table states
  12. delete snapshot 3, trigger SDT, verify table states
  13. delete snapshot 2, trigger SDT, verify table states
  14. delete snapshot 1, trigger SDT, verify table states
  15. delete snapshot 4, trigger SDT, verify table states

Parameterize for LEGACY, FSO and OBS.

Maybe in another jira.

@smengcl smengcl merged commit 4578a06 into apache:master May 2, 2023
27 checks passed
@smengcl
Copy link
Contributor

smengcl commented May 2, 2023

Thanks @ashishkumar50 for the patch. Thanks @devmadhuu and @ashishkumar50 for the reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
snapshot https://issues.apache.org/jira/browse/HDDS-6517
Projects
None yet
5 participants