HDDS-10156. Optimize Snapshot Cache get and eviction #6024

swamirishi · 2024-01-18T00:49:58Z

What changes were proposed in this pull request?

Currently SnapshotCache does a cleanup on every operation performed on the cache which is synchronized operation which overall makes the get,release operation non performant. A better approach would be introduce a background service which will periodically come up and perform a cleanup of all the unreferenced snapshot instances.

HDDS-10103 removed pending eviction list to avoid deadlock issues which means we have to check each and every snapshot instance in the cache if it has any references. If we use the concurrentHashSet for the pendingEvictionSet instead we can avoid such deadlock issues from occuring and it would be optimized at the same time.

What is the link to the Apache JIRA

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

How was this patch tested?

Adding unit tests and integration tests and existing tests should also help

hemantk-12

Thanks @swamirishi for working on this.

I looked at this briefly and overall looks good to me.

Left some inline comments.

...-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/ReferenceCounted.java

hemantk-12 · 2024-01-18T01:38:34Z

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

+    // TODO: [SNAPSHOT] Add OzoneManager.isLeaderReady() check along with
+    //  suspended. `isLeaderReady` check was removed because some unit tests
+    //  were failing due to Mockito limitation. Remove this once unit tests
+    //  or mocking are fixed.


Is this comment valid for this? Or just copy-paste from SnapshotDiffCleanupService?

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

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java

swagle · 2024-01-18T23:11:41Z

Why is this a Draft?

...op-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/freon/TestOMSnapshotDAG.java

aswinshakil · 2024-01-19T00:20:50Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java

-      dbMap.compute(entry.getKey(), (k, v) -> {
+    for (String evictionKey : pendingEvictionQueue) {
+      dbMap.compute(evictionKey, (k, v) -> {
+        pendingEvictionQueue.remove(k);


I'm not sure about this. Iterating and removing elements, while get() is adding to the same pendingEvictionQueue can cause ConcurrentModificationException

this should be fine, since this is a concurrent hashset.

Can we add test for it? Or may be use iterator.

aswinshakil · 2024-01-19T00:28:49Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java

   */
  public void invalidateAll() {
-    Iterator<Map.Entry<String, ReferenceCounted<IOmMetadataReader, SnapshotCache>>>
+    Iterator<Map.Entry<String, ReferenceCounted<IOmMetadataReader, SnapshotCache, String>>>


invalidateAll() and invalidate() should also cleanup the if entries already exist in pendingEvictionQueue.

swamirishi · 2024-01-19T18:27:36Z

Why is this a Draft?

I haven't added the unit test yet. I am working on it.

…anup instead of background service

hemantk-12 · 2024-02-06T19:35:05Z

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

+  private ReferenceCounted<IOmMetadataReader, SnapshotCache>
+      rcOmMetadataReader;


nit: it was indented better before.

hemantk-12 · 2024-02-06T20:21:23Z

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


-  public ReferenceCounted<
-      IOmMetadataReader, SnapshotCache> getOmMetadataReader() {
+  public ReferenceCounted<IOmMetadataReader,


nit: it could be in a single line since 120 chars are allowed per line now.

hemantk-12 · 2024-02-06T21:25:56Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java

    if (!skipActiveCheck && !omSnapshotManager.isSnapshotStatus(key, SNAPSHOT_ACTIVE)) {
      // Ref count was incremented. Need to decrement on exception here.
-      rcOmSnapshot.decrementRefCount();
+      rcOmSnapshot.close();


This should be decrementRefCount() and not close(). Some of the background services for exmaple SnapshotDeletingService works on non-Active snapshot. Snapshot is opened by SnapshotDeletingService, it will close the currently used snapshot.

Had a offline discussion, close() internal calls decrementRefCount().
But I think decrementRefCount() is better to use from readability perspective otherwise someone may have the same doubt I had.

hemantk-12 · 2024-02-06T21:26:17Z

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java

-      dbMap.compute(entry.getKey(), (k, v) -> {
+    for (String evictionKey : pendingEvictionQueue) {
+      dbMap.compute(evictionKey, (k, v) -> {
+        pendingEvictionQueue.remove(k);


Can we add test for it? Or may be use iterator.

hemantk-12 · 2024-02-06T21:30:06Z

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

    // Reset cache for each test case
    snapshotCache = new SnapshotCache(
-        omSnapshotManager, cacheLoader, CACHE_SIZE_LIMIT);
+        omSnapshotManager, cacheLoader, CACHE_SIZE_LIMIT, 0);


How setting the clean up interval to 0 is working for the test? I don't think it is correct to set clean-up to 0 or tests are more correct.

We don't want the cleanup service to run here, since everything is mocked here.

# 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/OmSnapshotManager.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/ReferenceCounted.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java # hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java # hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDiffManager.java

hemantk-12 · 2024-02-26T22:19:23Z

@swamirishi can you please fix the check-style and unit tests?

Change-Id: I79ba52de34a144e5e1d21c6a8c9f794ce8172e78

Change-Id: I3ea7fe97f0e6a5fc76387894ab1f56c87c6a78b1

Change-Id: Ic50b54d77a20c22ca4b55ec2e4ea647453ce87e5

Change-Id: I7d842af27926a434bd9b7da4197bc5e0330e3de6 # Conflicts: # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java # hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java # hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotDiffManager.java

Change-Id: If95fd3509b261f8ec5a1c72f7e8349742d17c6dd

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java

Change-Id: I3c59fc898deb9a95198cf383a5abde2b5d1f5f9f

Change-Id: I594b27798c9d7be9682ddaa8f0c3454592a0d8f3

hemantk-12

Thanks @swamirishi for the patch.
Left a comment about closing the scheduler otherwise looks good to me.

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

Change-Id: I9eeb49fe60851d6d58d55aa18682d63585320e7c

Change-Id: Id9bda88e9c79934f69712a8a93fb04c3fe74e3a4

hemantk-12

LGTM.

swamirishi · 2024-04-17T17:44:42Z

Thank you for the review @hemantk-12 @aswinshakil

(cherry picked from commit 3e97d8f)

…che#6024) (cherry picked from commit 3e97d8f)

HDDS-10156. Optimize Snapshot Cache get and eviction

f7b4354

swamirishi requested review from aswinshakil, hemantk-12 and smengcl January 18, 2024 00:50

swamirishi added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label Jan 18, 2024

hemantk-12 reviewed Jan 18, 2024

View reviewed changes

swagle reviewed Jan 18, 2024

View reviewed changes

...op-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/freon/TestOMSnapshotDAG.java Outdated Show resolved Hide resolved

aswinshakil requested changes Jan 19, 2024

View reviewed changes

swamirishi added 5 commits February 6, 2024 10:19

HDDS-10156. Remove Parent key and use scheduler for snapshotcachecle…

7140017

…anup instead of background service

HDDS-10156. Remove static

82f06f7

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

0045d82

HDDS-10156. Fix checkstyle

63c392d

HDDS-10156. Add config

fa5f165

hemantk-12 reviewed Feb 6, 2024

View reviewed changes

aswinshakil mentioned this pull request Feb 8, 2024

HDDS-10262. Encapsulate SnapshotCache inside OmSnapshotManager #6135

Merged

swamirishi and others added 3 commits February 20, 2024 12:33

HDDS-10156. Merge master

8254931

Merge branch 'master' into HDDS-10156

7df0ac9

swamirishi added 5 commits March 22, 2024 11:42

HDDS-10156. Fix checkstyle

e41e6d8

Change-Id: I79ba52de34a144e5e1d21c6a8c9f794ce8172e78

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

108d0ff

Change-Id: I3ea7fe97f0e6a5fc76387894ab1f56c87c6a78b1

HDDS-10156. Moved Cleanup Internal to cleanup function

a7cdcad

Change-Id: Ic50b54d77a20c22ca4b55ec2e4ea647453ce87e5

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

ab8727e

Change-Id: If95fd3509b261f8ec5a1c72f7e8349742d17c6dd

hemantk-12 reviewed Apr 12, 2024

View reviewed changes

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java Show resolved Hide resolved

swamirishi added 2 commits April 12, 2024 17:57

HDDS-10156. Fix testcases

a72cefb

Change-Id: I3c59fc898deb9a95198cf383a5abde2b5d1f5f9f

HDDS-10156. Fix testcases

0965244

Change-Id: I594b27798c9d7be9682ddaa8f0c3454592a0d8f3

hemantk-12 reviewed Apr 16, 2024

View reviewed changes

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java Outdated Show resolved Hide resolved

HDDS-10156. Fix snapshot cache close

15246fe

Change-Id: I9eeb49fe60851d6d58d55aa18682d63585320e7c

swamirishi marked this pull request as ready for review April 16, 2024 22:22

HDDS-10156. Invlaidate snapshots while closing snapshot cache

36742cd

Change-Id: Id9bda88e9c79934f69712a8a93fb04c3fe74e3a4

hemantk-12 approved these changes Apr 17, 2024

View reviewed changes

swamirishi merged commit 3e97d8f into apache:master Apr 17, 2024

jojochuang pushed a commit to jojochuang/ozone that referenced this pull request May 29, 2024

HDDS-10156. Optimize Snapshot Cache get and eviction (apache#6024)

6591706

(cherry picked from commit 3e97d8f)

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 17, 2024

HDDS-10156. Optimize Snapshot Cache get and eviction (apache#6024)

4a18564

(cherry picked from commit 3e97d8f)

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 17, 2024

HDDS-10156. Optimize Snapshot Cache get and eviction (apache#6024)

cd5334c

(cherry picked from commit 3e97d8f)

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 17, 2024

HDDS-10156. Optimize Snapshot Cache get and eviction (apache#6024)

7dc0ceb

(cherry picked from commit 3e97d8f)

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 18, 2024

HDDS-10156. Optimize Snapshot Cache get and eviction (apache#6024)

5111ebe

(cherry picked from commit 3e97d8f)

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 18, 2024

HDDS-10156. Optimize Snapshot Cache get and eviction (apache#6024)

9fc80a3

(cherry picked from commit 3e97d8f)

xichen01 mentioned this pull request Jul 18, 2024

[DO NOT MERGE] Backport some fixes, performance optimizations from master to ozone-1.4 #6929 #6964

Merged

swamirishi added a commit to swamirishi/ozone that referenced this pull request Dec 3, 2025

CDPD-65599. HDDS-10156. Optimize Snapshot Cache get and eviction (apa…

98ca8eb

…che#6024) (cherry picked from commit 3e97d8f)

		private ReferenceCounted<IOmMetadataReader, SnapshotCache>
		rcOmMetadataReader;

HDDS-10156. Optimize Snapshot Cache get and eviction #6024

HDDS-10156. Optimize Snapshot Cache get and eviction #6024

Uh oh!

Conversation

swamirishi commented Jan 18, 2024

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

swagle commented Jan 18, 2024

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!

swamirishi commented Jan 19, 2024

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 Feb 7, 2024 • 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!

hemantk-12 commented Feb 26, 2024

Uh oh!

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hemantk-12 left a comment

Choose a reason for hiding this comment

Uh oh!

swamirishi commented Apr 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hemantk-12 Feb 7, 2024 •

edited

Loading