HDDS-11603. Reclaimable Filter for Snapshots garbage reclaimation #7345

swamirishi · 2024-10-22T18:28:03Z

What changes were proposed in this pull request?

We have implemented the key reference check for snapshots in multiple places in different manner for snapshot garbage collection. There is a need to consolidate this reference check in one central place in order to avoid bugs and for better debuggability & for making the whole garbage collection more modular than it is. Currently the Garbage collection code is tightly coupled with snapshots, making it really tough for people to understand. We should move this code out to a different module making the garbage collection flow more readable.
To keep the patch size limited, the classes added are not used by any of the flows. I would be creating a follow up patches on top of this change.

What is the link to the Apache JIRA

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

How was this patch tested?

Unit Tests

Change-Id: I34b91e11970c9cd785ba863dd22a4081244cede6

Change-Id: I5dbb017343b5a666c56d53af2839dcb340fdf0ba

Change-Id: I67de73438c7ebc37c3d6e44bfe6e09188fb537e1

Change-Id: Ie80cd2191504e3a5ebd1cb21d04d300dc0c22ef9

Change-Id: If027d4ac4c5171ded3499b900909347d59406f36

Change-Id: I7fe6d4029abe5917e20bb82ae1b2494e2319da70

Change-Id: Ie7c8950b6511919a195cc31b677286621df4a198 # Conflicts: # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java # hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDiffManager.java

Change-Id: I7409c5f4b3ce256cdccfdf80aec0e439fc3fa48a

hemantk-12

Thanks, @swamirishi for the patch.

I reviewed it at high level. ReclaimableFilter approach looks alright to me but the implementation needs some improvement, especially the MultiLock. As I mentioned in one of the comments, it would be better to keep it close to garbage collection or snapshot and not provide too much flexibility or make it generic.

hemantk-12 · 2025-02-27T04:05:44Z

hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java

  String getRenameKey(String volume, String bucket, long objectID);

+  /**
+   * Given renameKey, return the volume, bucket and objectID from the key.


I don't think this is the correct place to keep this function but I'll let you decide it.

It seems ok because metadata manager is the translation layer b/w rocksdb & OM. We also have the function Given a volume name,bucket name & object give the renameTable key corresponding to the entry.

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/util/CheckedExceptionOperation.java

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/lock/MultiLocks.java

hemantk-12 · 2025-02-27T19:51:22Z

...zone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/filter/ReclaimableFilter.java

+    boolean isReclaimable = isReclaimable(keyValue);
+    // This is to ensure the reclamation ran on the same previous snapshot and no change occurred in the chain
+    // while processing the entry.
+    return isReclaimable && validateExistingLastNSnapshotsInChain(volume, bucket);


Since there is no usage of apply as of now, I don't know how it will be called. But it is weird to me that we have to do this volume and bucket check all the time as a precaution.

Yeah we would have to get the bucket & volume corresponding to the key. In case of AOS different keys corresponding buckets & volumes would be called.

hemantk-12 · 2025-02-27T19:52:50Z

...e-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/filter/ReclaimableKeyFilter.java

+  /**
+   * @param omSnapshotManager
+   * @param snapshotChainManager
+   * @param currentSnapshotInfo  : If null the deleted keys in AOS needs to be processed, hence the latest snapshot
+   *                             in the snapshot chain corresponding to bucket key needs to be processed.
+   * @param metadataManager      : MetadataManager corresponding to snapshot or AOS.
+   * @param lock                 : Lock for Active OM.
+   */


Same as other filters. Please keep them align or remove it.

hemantk-12 · 2025-02-27T21:04:48Z

...-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableFilter.java

+public class TestReclaimableFilter {
+
+  @Test
+  public void testReclaimableFilter() {


Is it just a placeholder? or you have plans to add more tests in the next revisions?

this is still work in progress.

hemantk-12 · 2025-02-27T21:06:59Z

...e-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/filter/ReclaimableKeyFilter.java

+    if (bucketInfo.getBucketLayout().isFileSystemOptimized()) {
+      dbKeyPrevSnap = ozoneManager.getMetadataManager().getOzonePathKey(
+          volumeId,
+          bucketInfo.getObjectID(),
+          keyInfo.getParentObjectID(),
+          keyInfo.getFileName());
+    } else {
+      dbKeyPrevSnap = ozoneManager.getMetadataManager().getOzoneKey(
+          keyInfo.getVolumeName(),
+          keyInfo.getBucketName(),
+          keyInfo.getKeyName());
+    }
+
+    String dbRenameKey = ozoneManager.getMetadataManager().getRenameKey(
+        keyInfo.getVolumeName(),
+        keyInfo.getBucketName(),
+        keyInfo.getObjectID());
+
+    String renamedKey = snapRenamedTable.getIfExist(dbRenameKey);


This is the same as 137-164 lines. Maybe create a helper function for it.

removed this redudant function altogether.

hemantk-12 · 2025-02-27T21:07:53Z

...e-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/filter/ReclaimableKeyFilter.java

+   *                 if key is exclusive to previousSnapshot.
+   */
+  @SuppressWarnings("checkstyle:ParameterNumber")
+  public void calculateExclusiveSize(


Should it be part of the reclaimable filter?

yeah we would want to calculate the exclusive size as part of the keyDeletingService run itself. Don't need a separate thread for this.

Change-Id: Icd0bb1a62e0435f4329cd1c0abdab52dce13827e

Change-Id: Ib8617005bbe1ac29835db8dcd6a27909cf08e57f

Change-Id: I8bab35cc6dfef7fcac5e69f806904049ff584e32

Change-Id: I81496bcab937444954a0c50d2790812c5412c14d

Change-Id: I2198239de160d7868758f6e06dc9658de10017ab # Conflicts: # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java

Change-Id: Ifc9ccb406af0aaf8f1282dfb2d612383dca102fb

Change-Id: I47390045a950428c29cc13469bc9639875b5f2e0

Change-Id: Id38661072466b08ab24140f13f3310f480711bd0

Change-Id: Ia1027f55775864c2353a3e67cc56b76cff5f0727

Change-Id: I6e4e5efe70366e45f15361f34914b4f313f8dd1a

Change-Id: If1a2f7670b2492a9d7295607e50f11c3bf92fb2f

adoroszlai · 2025-04-08T09:36:22Z

@swamirishi it seems this is split into #8053, #8054, #8055. Can we close this?

Change-Id: I6cae66db1a6f7f07d8a91849d1287567a75de633 # Conflicts: # hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/IOzoneManagerLock.java # hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/lock/OzoneManagerLock.java # hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/lock/TestOzoneManagerLock.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/MultiSnapshotLocks.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/filter/ReclaimableFilter.java # hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestMultiSnapshotLocks.java # hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/filter/TestReclaimableFilter.java

HDDS-11244. Fix Checkstyle

a9f3630

Change-Id: I34b91e11970c9cd785ba863dd22a4081244cede6

swamirishi changed the title ~~HDDS-11244. Reclaimable Filter for Snapshots garbage reclaimation~~ HDDS-11603. Reclaimable Filter for Snapshots garbage reclaimation Oct 22, 2024

swamirishi requested review from aswinshakil and hemantk-12 October 23, 2024 22:33

swamirishi requested a review from prashantpogde January 17, 2025 07:00

swamirishi added 7 commits January 17, 2025 07:50

HDDS-11603. Fix checkstyle

80713fd

Change-Id: I5dbb017343b5a666c56d53af2839dcb340fdf0ba

HDDS-11603. Fix checkstyle

dcfec3d

Change-Id: I67de73438c7ebc37c3d6e44bfe6e09188fb537e1

Merge remote-tracking branch 'apache/master' into HEAD

e2a8cfe

Change-Id: Ie80cd2191504e3a5ebd1cb21d04d300dc0c22ef9

HDDS-11603. Fix javadoc

7096a1c

Change-Id: If027d4ac4c5171ded3499b900909347d59406f36

HDDS-11603. Fix test

7514ac3

Change-Id: I7fe6d4029abe5917e20bb82ae1b2494e2319da70

HDDS-11603. Fix checkstyle

7c141fd

Change-Id: I7409c5f4b3ce256cdccfdf80aec0e439fc3fa48a

hemantk-12 reviewed Feb 27, 2025

View reviewed changes

swamirishi added 11 commits March 6, 2025 02:37

HDDS-11603. Refactor code simplify logic

acdaffe

Change-Id: Icd0bb1a62e0435f4329cd1c0abdab52dce13827e

HDDS-11603. Fix checkstyle

b94a54f

Change-Id: Ib8617005bbe1ac29835db8dcd6a27909cf08e57f

HDDS-11603. Address review comments

df8c562

Change-Id: I8bab35cc6dfef7fcac5e69f806904049ff584e32

HDDS-11603. Remove over nesting on snapshot initialize in filter

8b7c738

Change-Id: I81496bcab937444954a0c50d2790812c5412c14d

Merge remote-tracking branch 'apache/master' into HEAD

fdafee8

Change-Id: I2198239de160d7868758f6e06dc9658de10017ab # Conflicts: # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotDiffManager.java

HDDS-11603. Fix size calculation issue

8bc3cc6

Change-Id: Ifc9ccb406af0aaf8f1282dfb2d612383dca102fb

HDDS-11603. Make more modular

4c9bdfb

Change-Id: I47390045a950428c29cc13469bc9639875b5f2e0

HDDS-11603. Add reclaimable Filter test case

b7e20ca

Change-Id: Id38661072466b08ab24140f13f3310f480711bd0

HDDS-11603. Add KeyManager test

0ee6b06

Change-Id: Ia1027f55775864c2353a3e67cc56b76cff5f0727

Merge remote-tracking branch 'apache/master' into HEAD

890eeb4

Change-Id: I6e4e5efe70366e45f15361f34914b4f313f8dd1a

Merge remote-tracking branch 'apache/master' into HEAD

0d209b7

Change-Id: If1a2f7670b2492a9d7295607e50f11c3bf92fb2f

ivandika3 added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Mar 21, 2025

swamirishi closed this Apr 8, 2025

swamirishi reopened this Apr 30, 2025

adoroszlai closed this May 1, 2025

swamirishi reopened this May 5, 2025

swamirishi closed this May 5, 2025

HDDS-11603. Reclaimable Filter for Snapshots garbage reclaimation #7345

HDDS-11603. Reclaimable Filter for Snapshots garbage reclaimation #7345

Uh oh!

Conversation

swamirishi commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

hemantk-12 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!

Uh oh!

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!

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!

adoroszlai commented Apr 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

swamirishi commented Oct 22, 2024 •

edited

Loading