You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "swamirishi (via GitHub)" <gi...@apache.org> on 2024/01/18 00:50:01 UTC
[PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
swamirishi opened a new pull request, #6024:
URL: https://github.com/apache/ozone/pull/6024
## 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](https://issues.apache.org/jira/browse/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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1481955018
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -178,18 +203,12 @@ public ReferenceCounted<IOmMetadataReader, SnapshotCache> get(String key, boolea
// when called from SDT (and some) if the snapshot is not active anymore.
if (!skipActiveCheck && !omSnapshotManager.isSnapshotStatus(key, SNAPSHOT_ACTIVE)) {
// Ref count was incremented. Need to decrement on exception here.
- rcOmSnapshot.decrementRefCount();
+ rcOmSnapshot.close();
Review Comment:
Had a offline discussion, `close()` internal calls `decrementRefCount()`.
But I think `decrementRefCount()` is better to use from readability perspective otherwise someone will have the same doubt I had.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -178,18 +203,12 @@ public ReferenceCounted<IOmMetadataReader, SnapshotCache> get(String key, boolea
// when called from SDT (and some) if the snapshot is not active anymore.
if (!skipActiveCheck && !omSnapshotManager.isSnapshotStatus(key, SNAPSHOT_ACTIVE)) {
// Ref count was incremented. Need to decrement on exception here.
- rcOmSnapshot.decrementRefCount();
+ rcOmSnapshot.close();
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1568041077
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -418,6 +426,7 @@ public int getSnapshotCacheSize() {
public void invalidateCache() {
if (snapshotCache != null) {
snapshotCache.invalidateAll();
+ snapshotCache.close();
Review Comment:
nit: I just realized if it would be better to call `snapshotCache.invalidateAll()` from inside of `snapshotCache.close()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1563293380
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -185,58 +202,58 @@ public ReferenceCounted<OmSnapshot> get(UUID key) throws IOException {
* @param key SnapshotId
*/
public void release(UUID key) {
- dbMap.compute(key, (k, v) -> {
- if (v == null) {
- throw new IllegalArgumentException("SnapshotId '" + key + "' does not exist in cache.");
- } else {
- v.decrementRefCount();
- }
- return v;
- });
-
- // The cache size might have already exceeded the soft limit
- // Thus triggering cleanup() to check and evict if applicable
- cleanup();
+ ReferenceCounted<OmSnapshot> val = dbMap.get(key);
+ if (val == null) {
+ throw new IllegalArgumentException("Key '" + key + "' does not " +
+ "exist in cache.");
+ }
+ val.decrementRefCount();
}
/**
- * Wrapper for cleanupInternal() that is synchronized to prevent multiple
- * threads from interleaving into the cleanup method.
+ * If cache size exceeds soft limit, attempt to clean up and close the
+ instances that has zero reference count.
*/
- private synchronized void cleanup() {
+ private void cleanup() {
if (dbMap.size() > cacheSizeLimit) {
Review Comment:
Since this is a separate task, do we need this check? We can simply evict the pending keys irrespective if dbMap's size reaches the soft limit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi merged PR #6024:
URL: https://github.com/apache/ozone/pull/6024
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1567999435
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -418,6 +426,7 @@ public int getSnapshotCacheSize() {
public void invalidateCache() {
if (snapshotCache != null) {
snapshotCache.invalidateAll();
+ snapshotCache.close();
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1480558731
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -237,8 +242,9 @@ private synchronized void cleanup() {
* TODO: [SNAPSHOT] Add new ozone debug CLI command to trigger this directly.
*/
private void cleanupInternal() {
- for (Map.Entry<String, ReferenceCounted<IOmMetadataReader, SnapshotCache>> entry : dbMap.entrySet()) {
- dbMap.compute(entry.getKey(), (k, v) -> {
+ for (String evictionKey : pendingEvictionQueue) {
+ dbMap.compute(evictionKey, (k, v) -> {
+ pendingEvictionQueue.remove(k);
Review Comment:
Can we add test for it? Or may be use iterator.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java:
##########
@@ -82,7 +82,7 @@ static void beforeAll() throws Exception {
void setUp() {
// Reset cache for each test case
snapshotCache = new SnapshotCache(
- omSnapshotManager, cacheLoader, CACHE_SIZE_LIMIT);
+ omSnapshotManager, cacheLoader, CACHE_SIZE_LIMIT, 0);
Review Comment:
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.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -485,7 +485,8 @@ private enum State {
private OmMetadataReader omMetadataReader;
// Wrap active DB metadata reader in ReferenceCounted once to avoid
// instance creation every single time.
- private ReferenceCounted<IOmMetadataReader, SnapshotCache> rcOmMetadataReader;
+ private ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ rcOmMetadataReader;
Review Comment:
nit: it was indented better before.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -178,18 +203,12 @@ public ReferenceCounted<IOmMetadataReader, SnapshotCache> get(String key, boolea
// when called from SDT (and some) if the snapshot is not active anymore.
if (!skipActiveCheck && !omSnapshotManager.isSnapshotStatus(key, SNAPSHOT_ACTIVE)) {
// Ref count was incremented. Need to decrement on exception here.
- rcOmSnapshot.decrementRefCount();
+ rcOmSnapshot.close();
Review Comment:
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.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -2580,8 +2581,8 @@ public boolean getAllowListAllVolumes() {
return allowListAllVolumes;
}
- public ReferenceCounted<
- IOmMetadataReader, SnapshotCache> getOmMetadataReader() {
+ public ReferenceCounted<IOmMetadataReader,
Review Comment:
nit: it could be in a single line since 120 chars are allowed per line now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1536049089
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -221,11 +213,24 @@ public void release(OmSnapshot omSnapshot) {
release(snapshotTableKey);
}
+ /**
+ * Callback method used to enqueue or dequeue ReferenceCounted from
+ * pendingEvictionList.
+ * @param referenceCounted ReferenceCounted object
+ */
+ @Override
+ public void callback(ReferenceCounted referenceCounted) {
+ if (referenceCounted.getTotalRefCount() == 0L) {
+ // Reference count reaches zero, add to pendingEvictionList
+ pendingEvictionQueue.add((String) referenceCounted.getParentKey());
+ }
+ }
+
/**
* Wrapper for cleanupInternal() that is synchronized to prevent multiple
* threads from interleaving into the cleanup method.
*/
- private synchronized void cleanup() {
+ public synchronized void cleanup() {
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1563348210
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -185,58 +202,58 @@ public ReferenceCounted<OmSnapshot> get(UUID key) throws IOException {
* @param key SnapshotId
*/
public void release(UUID key) {
- dbMap.compute(key, (k, v) -> {
- if (v == null) {
- throw new IllegalArgumentException("SnapshotId '" + key + "' does not exist in cache.");
- } else {
- v.decrementRefCount();
- }
- return v;
- });
-
- // The cache size might have already exceeded the soft limit
- // Thus triggering cleanup() to check and evict if applicable
- cleanup();
+ ReferenceCounted<OmSnapshot> val = dbMap.get(key);
+ if (val == null) {
+ throw new IllegalArgumentException("Key '" + key + "' does not " +
+ "exist in cache.");
+ }
+ val.decrementRefCount();
}
/**
- * Wrapper for cleanupInternal() that is synchronized to prevent multiple
- * threads from interleaving into the cleanup method.
+ * If cache size exceeds soft limit, attempt to clean up and close the
+ instances that has zero reference count.
*/
- private synchronized void cleanup() {
+ private void cleanup() {
if (dbMap.size() > cacheSizeLimit) {
Review Comment:
We can have the soft limit in place. This would ensure that we don't close snapshots if it is less than soft limit. This would ensure keep at least a few snapshots open and just do optimistic closing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "swagle (via GitHub)" <gi...@apache.org>.
swagle commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1458071509
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/freon/TestOMSnapshotDAG.java:
##########
@@ -213,11 +213,11 @@ public void testDAGReconstruction()
OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
RDBStore rdbStore = (RDBStore) omMetadataManager.getStore();
RocksDBCheckpointDiffer differ = rdbStore.getRocksDBCheckpointDiffer();
- ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ ReferenceCounted<IOmMetadataReader, SnapshotCache, String>
snapDB1 = ozoneManager.getOmSnapshotManager()
.getSnapshotCache().get(
SnapshotInfo.getTableKey(volumeName, bucketName, "snap1"));
- ReferenceCounted<IOmMetadataReader, SnapshotCache>
+ ReferenceCounted<IOmMetadataReader, SnapshotCache, String>
Review Comment:
Test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #6024:
URL: https://github.com/apache/ozone/pull/6024#issuecomment-1965403448
@swamirishi can you please fix the check-style and unit tests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1536040856
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/ReferenceCounted.java:
##########
@@ -52,9 +52,10 @@ public class ReferenceCounted<T, U>
* Parent instance whose callback will be triggered upon this RC closure.
*/
private final U parentWithCallback;
+ private final K parentKey;
Review Comment:
done
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotCacheCleanupService.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.service;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.hadoop.hdds.utils.BackgroundService;
+import org.apache.hadoop.hdds.utils.BackgroundTask;
+import org.apache.hadoop.hdds.utils.BackgroundTaskQueue;
+import org.apache.hadoop.hdds.utils.BackgroundTaskResult;
+import org.apache.hadoop.ozone.om.snapshot.SnapshotCache;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * Background service to clean-up snapshot cache which would cleanup
+ * unreferenced snapshot instances.
+ */
+public class SnapshotCacheCleanupService extends BackgroundService {
+ // Use only a single thread for Snapshot Cache cleanup.
+ private static final int SNAPSHOT_CACHE_CLEANUP_CORE_POOL_SIZE = 1;
+
+ private final AtomicBoolean suspended;
+ private final AtomicLong runCount;
+ private final AtomicLong successRunCount;
+
+ private final SnapshotCache snapshotCache;
+
+ public SnapshotCacheCleanupService(long interval,
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1480359691
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -237,8 +242,9 @@ private synchronized void cleanup() {
* TODO: [SNAPSHOT] Add new ozone debug CLI command to trigger this directly.
*/
private void cleanupInternal() {
- for (Map.Entry<String, ReferenceCounted<IOmMetadataReader, SnapshotCache>> entry : dbMap.entrySet()) {
- dbMap.compute(entry.getKey(), (k, v) -> {
+ for (String evictionKey : pendingEvictionQueue) {
+ dbMap.compute(evictionKey, (k, v) -> {
+ pendingEvictionQueue.remove(k);
Review Comment:
this should be fine, since this is a concurrent hashset.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "swagle (via GitHub)" <gi...@apache.org>.
swagle commented on PR #6024:
URL: https://github.com/apache/ozone/pull/6024#issuecomment-1899364170
Why is this a Draft?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1458135681
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -98,11 +102,11 @@ public void invalidate(String key) throws IOException {
* Immediately invalidate all entries and close their DB instances in cache.
*/
public void invalidateAll() {
- Iterator<Map.Entry<String, ReferenceCounted<IOmMetadataReader, SnapshotCache>>>
+ Iterator<Map.Entry<String, ReferenceCounted<IOmMetadataReader, SnapshotCache, String>>>
Review Comment:
`invalidateAll()` and `invalidate()` should also cleanup the if entries already exist in `pendingEvictionQueue`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -237,8 +242,9 @@ private synchronized void cleanup() {
* TODO: [SNAPSHOT] Add new ozone debug CLI command to trigger this directly.
*/
private void cleanupInternal() {
- for (Map.Entry<String, ReferenceCounted<IOmMetadataReader, SnapshotCache>> entry : dbMap.entrySet()) {
- dbMap.compute(entry.getKey(), (k, v) -> {
+ for (String evictionKey : pendingEvictionQueue) {
+ dbMap.compute(evictionKey, (k, v) -> {
+ pendingEvictionQueue.remove(k);
Review Comment:
I'm not sure about this. Iterating and removing elements, while` get()` is adding to the same `pendingEvictionQueue` can cause `ConcurrentModificationException`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1456710700
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/ReferenceCounted.java:
##########
@@ -52,9 +52,10 @@ public class ReferenceCounted<T, U>
* Parent instance whose callback will be triggered upon this RC closure.
*/
private final U parentWithCallback;
+ private final K parentKey;
Review Comment:
1. It would be great if you can add the java doc comment for this.
2. nit: If it is just use to get the name of the snapshot (key for the snapshot object) [here](https://github.com/apache/ozone/pull/6024/files#diff-d5b0b24274c5d19e876dc2ffeb744d4965fa1a23ce727ac09b965f665573c18aR225), I'll suggest to use `((OmSnapshot) referenceCounted.get()).getSnapshotTableKey()` there rather than adding new parameter.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -221,11 +213,24 @@ public void release(OmSnapshot omSnapshot) {
release(snapshotTableKey);
}
+ /**
+ * Callback method used to enqueue or dequeue ReferenceCounted from
+ * pendingEvictionList.
+ * @param referenceCounted ReferenceCounted object
+ */
+ @Override
+ public void callback(ReferenceCounted referenceCounted) {
+ if (referenceCounted.getTotalRefCount() == 0L) {
+ // Reference count reaches zero, add to pendingEvictionList
+ pendingEvictionQueue.add((String) referenceCounted.getParentKey());
+ }
+ }
+
/**
* Wrapper for cleanupInternal() that is synchronized to prevent multiple
* threads from interleaving into the cleanup method.
*/
- private synchronized void cleanup() {
+ public synchronized void cleanup() {
Review Comment:
nit:
1. I think we can remove `synchronized` now.
2. `cleanupInternal` doesn't have to be separate function. We can remove it and move the code to `cleanup`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotCacheCleanupService.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.service;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.hadoop.hdds.utils.BackgroundService;
+import org.apache.hadoop.hdds.utils.BackgroundTask;
+import org.apache.hadoop.hdds.utils.BackgroundTaskQueue;
+import org.apache.hadoop.hdds.utils.BackgroundTaskResult;
+import org.apache.hadoop.ozone.om.snapshot.SnapshotCache;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * Background service to clean-up snapshot cache which would cleanup
+ * unreferenced snapshot instances.
+ */
+public class SnapshotCacheCleanupService extends BackgroundService {
+ // Use only a single thread for Snapshot Cache cleanup.
+ private static final int SNAPSHOT_CACHE_CLEANUP_CORE_POOL_SIZE = 1;
+
+ private final AtomicBoolean suspended;
+ private final AtomicLong runCount;
+ private final AtomicLong successRunCount;
+
+ private final SnapshotCache snapshotCache;
+
+ public SnapshotCacheCleanupService(long interval,
+ long serviceTimeout,
+ String threadPrefix,
+ SnapshotCache snapshotCache) {
+ super(SnapshotCacheCleanupService.class.getSimpleName(),
+ interval,
+ TimeUnit.MILLISECONDS, SNAPSHOT_CACHE_CLEANUP_CORE_POOL_SIZE,
+ serviceTimeout, threadPrefix);
+ this.suspended = new AtomicBoolean(false);
+ this.runCount = new AtomicLong(0);
+ this.successRunCount = new AtomicLong(0);
+ this.snapshotCache = snapshotCache;
+
+ }
+
+ @VisibleForTesting
+ public void run() {
+ snapshotCache.cleanup();
+ }
+
+ private class SnapshotCacheCleanUpTask implements BackgroundTask {
+
+ @Override
+ public BackgroundTaskResult call() {
+ if (!shouldRun()) {
+ return BackgroundTaskResult.EmptyTaskResult.newResult();
+ }
+ runCount.incrementAndGet();
+ run();
+ return BackgroundTaskResult.EmptyTaskResult.newResult();
+ }
+ }
+
+ @Override
+ public BackgroundTaskQueue getTasks() {
+ BackgroundTaskQueue queue = new BackgroundTaskQueue();
+ queue.add(new SnapshotCacheCleanUpTask());
+ return queue;
+ }
+
+ private boolean shouldRun() {
+ // 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.
Review Comment:
Is this comment valid for this? Or just copy-paste from `SnapshotDiffCleanupService`?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotCacheCleanupService.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.service;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.hadoop.hdds.utils.BackgroundService;
+import org.apache.hadoop.hdds.utils.BackgroundTask;
+import org.apache.hadoop.hdds.utils.BackgroundTaskQueue;
+import org.apache.hadoop.hdds.utils.BackgroundTaskResult;
+import org.apache.hadoop.ozone.om.snapshot.SnapshotCache;
+
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * Background service to clean-up snapshot cache which would cleanup
+ * unreferenced snapshot instances.
+ */
+public class SnapshotCacheCleanupService extends BackgroundService {
+ // Use only a single thread for Snapshot Cache cleanup.
+ private static final int SNAPSHOT_CACHE_CLEANUP_CORE_POOL_SIZE = 1;
+
+ private final AtomicBoolean suspended;
+ private final AtomicLong runCount;
+ private final AtomicLong successRunCount;
+
+ private final SnapshotCache snapshotCache;
+
+ public SnapshotCacheCleanupService(long interval,
Review Comment:
nit (suggestion): I think we don't need all the functionality of `BackgroundService` for this. I would prefer to have a simple daemon thread in [Scheduler](https://github.com/apache/ozone/blob/3964ece6561b0402165c4158d51b65e9d82c8860/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/Scheduler.java#L32) similar to background cleaner in [RocksDBCheckpointDiffer](https://github.com/apache/ozone/blob/6d41b8709b8fd81405cc06b3847c8989c60a80ab/hadoop-hdds/rocksdb-checkpoint-differ/src/main/java/org/apache/ozone/rocksdiff/RocksDBCheckpointDiffer.java#L233).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1567879432
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -418,6 +426,7 @@ public int getSnapshotCacheSize() {
public void invalidateCache() {
if (snapshotCache != null) {
snapshotCache.invalidateAll();
+ snapshotCache.close();
Review Comment:
I think it should be closed inside `OmSnapshotManager#close()` because invalidateCache is also called by [OzoneManager#installCheckpoint](https://github.com/apache/ozone/blob/9efbccea8aaf0aaa14686b53927a64c4d13118a0/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L3788) and I don't think we want to close scheduler in that case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1568056367
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -418,6 +426,7 @@ public int getSnapshotCacheSize() {
public void invalidateCache() {
if (snapshotCache != null) {
snapshotCache.invalidateAll();
+ snapshotCache.close();
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #6024:
URL: https://github.com/apache/ozone/pull/6024#issuecomment-2061866801
Thank you for the review @hemantk-12 @aswinshakil
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1458108560
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/ReferenceCounted.java:
##########
@@ -52,9 +52,10 @@ public class ReferenceCounted<T, U>
* Parent instance whose callback will be triggered upon this RC closure.
*/
private final U parentWithCallback;
+ private final K parentKey;
Review Comment:
I agree with the 2nd point. There is no need to have a separate `parentKey` we can just use `((OmSnapshot) referenceCounted.get()).getSnapshotTableKey()` to get the key. I fell like this is only adding more things to change in the codebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #6024:
URL: https://github.com/apache/ozone/pull/6024#issuecomment-1900900314
> Why is this a Draft?
I haven't added the unit test yet. I am working on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10156. Optimize Snapshot Cache get and eviction [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #6024:
URL: https://github.com/apache/ozone/pull/6024#discussion_r1536023265
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java:
##########
@@ -82,7 +82,7 @@ static void beforeAll() throws Exception {
void setUp() {
// Reset cache for each test case
snapshotCache = new SnapshotCache(
- omSnapshotManager, cacheLoader, CACHE_SIZE_LIMIT);
+ omSnapshotManager, cacheLoader, CACHE_SIZE_LIMIT, 0);
Review Comment:
We don't want the cleanup service to run here, since everything is mocked here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org