HDDS-9312. Expand all the entries deletedDirTable for Snapshot. #5339

aswinshakil · 2023-09-20T22:23:22Z

What changes were proposed in this pull request?

Currently, deleted directories are kept in snapshot's deletedDirTable. It doesn't have information on the total size of the keys it holds. So we won't be able to get the exclusiveSize of the snapshot unless we expand it. This PR expands the deledDirTable of all the snapshots.

What is the link to the Apache JIRA

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

How was this patch tested?

Added Integration Test.

aswinshakil · 2023-09-20T22:23:47Z

cc: @swamirishi

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/SnapshotInfo.java

hemantk-12 · 2023-09-27T17:13:43Z

hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmSnapshotInfo.java

        snapshotInfoActual.getExclusiveSize());
    Assert.assertEquals(snapshotInfoExpected.getExclusiveReplicatedSize(),
        snapshotInfoActual.getExclusiveReplicatedSize());
+    Assert.assertEquals(snapshotInfoExpected.getExpandedDeletedDir(),


Can we just assertEquals on snapshotInfoExpected and snapshotInfoActual?

Yes we can, but this is just to check each field individually.

assertEquals() will do the same thing as long as equals method is overridden.
https://github.com/junit-team/junit4/blob/main/src/main/java/org/junit/Assert.java#L145

...ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java

hemantk-12 · 2023-09-27T17:30:01Z

...ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java

+            }
+
+            Table.KeyValue<String, OmKeyInfo> deletedDirInfo;
+            List<Pair<String, OmKeyInfo>> allSubDirList


nit: Is there any specific reason it is list of Pair? If no, use map instead for simplification.

This is also a copy-pasta from the DirectoryDeletingService. This was the structure used by the service all this time. So I kept it this way.

hemantk-12 · 2023-09-27T17:34:44Z

...ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java

+            List<Pair<String, OmKeyInfo>> allSubDirList
+                = new ArrayList<>((int) remainNum);
+
+            try (TableIterator<String, ? extends Table.KeyValue<String,


Code inside this try block is mostly duplicate of above try block. Duplication can be removed, if you create a helper function and extract this out. It will also improve the readability.

We can't fully extract it because we use some snapshot-specific code in-between. So I kept it separately.

https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java#L180-L216 and https://github.com/apache/ozone/pull/5339/files#diff-7ec30412dac9f4304cc72760eec9efe605e901ec3a7d379551da7c6ab67ea7eaR294-R331 exactly same except pendingDeletedDirInfo changed to deletedDirInfo which I think is just variable name change. I believe you can extract you the table entry processing.

One more point, we are not iterating over deletedIterator? Is it intentional? Also do we need to call deletedIterator.seekToFirst(); and deletedIterator.hasNext(); before calling deletedIterator.next(); ?

Correct I missed that part with copy-pasting, I have updated the PR.

hemantk-12 · 2023-09-27T17:35:53Z

...ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java

+          if (remainNum > 0) {
+            expandSnapshotDirectories(remainNum);
+          }
+        } catch (Exception e) {


I think you can just catch this exception inside expandSnapshotDirectories and log there instead of logging it here.

expandSnapshotDirectories already has its own separate logging for something similar.

Then it is double logging. May be expandSnapshotDirectories doesn't need to throw an IOException.

It will throw because we still do get from RocksDB. But the exception on inside of expandSnapshotDirectories is specific for iterating through snapDeletedDirTable. The one outside is overall exeption. The main reason is expandSnapshotDirectories already las many try blocks, adding more will decrease the readability.

…12-direxpand

swamirishi

@aswinshakil thanks for the patch. Have a few review comments from the first round of review. I will add more comments as and when I get more clarity on the whole thing.

swamirishi · 2023-10-12T18:28:37Z

...ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java

+
+          // Expand deleted dirs only on active snapshot. Deleted Snapshots
+          // will be cleaned up by SnapshotDeletingService.
+          if (currSnapInfo.getSnapshotStatus() != SNAPSHOT_ACTIVE ||


We should take a lock on this snapshot to avoid race condition

swamirishi · 2023-10-12T18:30:09Z

...ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java

+                "null for snapshotted bucket. The OM is in unexpected state.");
+          }
+
+          String dbBucketKeyForDir = getOzoneManager().getMetadataManager()


Can we have some kind of a util function for this? or use one if it is already there. As in appending OM_KEY_PREFIX at the end.

swamirishi · 2023-10-12T18:31:58Z

...ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java

+              currSnapInfo.getVolumeName(),
+              currSnapInfo.getBucketName(),
+              getSnapshotPrefix(currSnapInfo.getName()),
+              true)) {


I guess this should be false here. skipActiveCheck has to be false.

swamirishi · 2023-10-12T18:54:26Z

...ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java

+                subFileNum += request.getDeletedSubFilesCount();
+              }
+
+              optimizeDirDeletesAndSubmitRequest(


From what I understand this would put a request to expand the directory and manipulate the directory table and key table entries. I am not sure how this would impact snapshot diff. @hemantk-12 @prashantpogde .

Full diff and Dag based diff will start showing different outputs.

aswinshakil · 2023-10-19T18:05:18Z

We have discussed a different approach to not modify keyTable and dirTable as it will change the snapDiff.

HDDS-9312. Expand all the entries deletedDirTable for Snapshot.

56b2ab1

aswinshakil added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Sep 20, 2023

aswinshakil self-assigned this Sep 20, 2023

aswinshakil requested a review from hemantk-12 September 20, 2023 22:23

aswinshakil added 2 commits September 20, 2023 16:26

HDDS-9312. Fix Findbugs.

5f4bbf3

Fix TestOmSnapshotInfo unit test

61e9376

hemantk-12 reviewed Sep 27, 2023

View reviewed changes

aswinshakil added 8 commits September 27, 2023 16:00

Address review comments.

3f74baa

Merge master

382962d

Fix deletedIterator logic.

da3f654

Fix checkstyle.

d647921

Merge branch 'master' of https://github.com/apache/ozone into HDDS-93…

cd82700

…12-direxpand

Fix testSnapshotWithFSO and remove move directories code.

8fc3a25

Merge branch 'master' of https://github.com/apache/ozone into HDDS-93…

75f3f5e

…12-direxpand

Fix findbugs

9baa010

swamirishi requested changes Oct 12, 2023

View reviewed changes

Modify KDT and DDT

fb4aca2

aswinshakil closed this Oct 19, 2023

This was referenced Nov 8, 2023

HDDS-9636. Fix NoSuchElement in OMSnapshotPurgeRequest and exit loop early in SnapshotDeletingService #5554

Merged

HDDS-9426. Calculate Exclusive size for deep cleaned snapshot's deleted directories. #5579

Merged

aswinshakil mentioned this pull request Jan 26, 2024

HDDS-9426. Calculate Exclusive size for deep cleaned snapshot's deleted directories #6099

Merged

HDDS-9312. Expand all the entries deletedDirTable for Snapshot. #5339

HDDS-9312. Expand all the entries deletedDirTable for Snapshot. #5339

Uh oh!

Conversation

aswinshakil commented Sep 20, 2023

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

aswinshakil commented Sep 20, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hemantk-12 Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swamirishi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aswinshakil commented Oct 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hemantk-12 Sep 27, 2023 •

edited

Loading