Copilot commented on code in PR #9869:
URL: https://github.com/apache/ozone/pull/9869#discussion_r2902951931
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -377,26 +383,26 @@ private synchronized Void cleanup(UUID evictionKey,
boolean expectKeyToBePresent
}
dbMap.compute(evictionKey, (k, v) -> {
- pendingEvictionQueue.remove(k);
+ ReferenceCounted<OmSnapshot> result = null;
if (v == null) {
- throw new IllegalStateException("SnapshotId '" + k + "' does not exist
in cache. The RocksDB " +
+ LOG.warn("SnapshotId '" + k + "' does not exist in cache. The RocksDB
" +
"instance of the Snapshot may not be closed properly.");
Review Comment:
The new warning for stale eviction keys (v == null) says the RocksDB
instance “may not be closed properly”, but this condition can also occur in the
expected invalidate + late close race (snapshot was closed and removed from
dbMap, and the callback re-queued the UUID). Consider rewording the log to
reflect that this can be a benign stale-queue entry, and include guidance only
if it indicates a real leak signal.
```suggestion
LOG.warn("SnapshotId '{}' not found in cache during cleanup. "
+ "This can happen if the snapshot was already closed and "
+ "removed from the cache, leaving a stale entry in the eviction
"
+ "queue. If this message appears frequently for the same "
+ "snapshot or the cache size keeps growing, it may indicate
that "
+ "a RocksDB instance was not closed properly.");
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -377,26 +383,26 @@ private synchronized Void cleanup(UUID evictionKey,
boolean expectKeyToBePresent
}
dbMap.compute(evictionKey, (k, v) -> {
- pendingEvictionQueue.remove(k);
+ ReferenceCounted<OmSnapshot> result = null;
if (v == null) {
- throw new IllegalStateException("SnapshotId '" + k + "' does not exist
in cache. The RocksDB " +
+ LOG.warn("SnapshotId '" + k + "' does not exist in cache. The RocksDB
" +
"instance of the Snapshot may not be closed properly.");
- }
-
- if (v.getTotalRefCount() > 0) {
+ } else if (v.getTotalRefCount() > 0) {
LOG.debug("SnapshotId {} is still being referenced ({}), skipping its
clean up.", k, v.getTotalRefCount());
- return v;
+ result = v;
} else {
LOG.debug("Closing SnapshotId {}. It is not being referenced
anymore.", k);
// Close the instance, which also closes its DB handle.
try {
v.get().close();
} catch (IOException ex) {
- throw new IllegalStateException("Error while closing snapshot DB.",
ex);
+ LOG.error("Error while closing snapshot DB.", ex);
+ return v;
}
omMetrics.decNumSnapshotCacheSize();
- return null;
}
+ pendingEvictionQueue.remove(k);
+ return result;
});
Review Comment:
In cleanup(evictionKey, ...), the pending eviction key is removed even when
the snapshot is still referenced (totalRefCount > 0). If another thread
decrements the refcount to 0 and the callback re-adds the key concurrently,
this removal can win and drop the key permanently, leaving an unreferenced
snapshot in dbMap that will never be retried for cleanup. Consider either not
removing the key when refcount > 0, or removing it conditionally and re-adding
if the refcount becomes 0 after the decision (or otherwise synchronizing queue
updates).
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -377,26 +383,26 @@ private synchronized Void cleanup(UUID evictionKey,
boolean expectKeyToBePresent
}
dbMap.compute(evictionKey, (k, v) -> {
- pendingEvictionQueue.remove(k);
+ ReferenceCounted<OmSnapshot> result = null;
if (v == null) {
- throw new IllegalStateException("SnapshotId '" + k + "' does not exist
in cache. The RocksDB " +
+ LOG.warn("SnapshotId '" + k + "' does not exist in cache. The RocksDB
" +
"instance of the Snapshot may not be closed properly.");
- }
-
- if (v.getTotalRefCount() > 0) {
+ } else if (v.getTotalRefCount() > 0) {
LOG.debug("SnapshotId {} is still being referenced ({}), skipping its
clean up.", k, v.getTotalRefCount());
- return v;
+ result = v;
} else {
LOG.debug("Closing SnapshotId {}. It is not being referenced
anymore.", k);
// Close the instance, which also closes its DB handle.
try {
v.get().close();
} catch (IOException ex) {
- throw new IllegalStateException("Error while closing snapshot DB.",
ex);
+ LOG.error("Error while closing snapshot DB.", ex);
Review Comment:
The close-failure log message loses the snapshot context (it only logs a
generic "Error while closing snapshot DB"). Including the snapshotId/eviction
key in this log would make diagnosing repeated cleanup retries much easier.
```suggestion
LOG.error("Error while closing snapshot DB for snapshotId {}.", k,
ex);
```
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java:
##########
@@ -511,4 +515,147 @@ void testSnapshotOperationsNotBlockedDuringCompaction()
throws IOException, Inte
verify(store1, times(1)).compactTable("table2");
verify(store1, times(0)).compactTable("keyTable");
}
+
+ @SuppressWarnings("unchecked")
+ private static Set<UUID> getPendingEvictionQueue(SnapshotCache cache) {
+ try {
+ Field f = SnapshotCache.class.getDeclaredField("pendingEvictionQueue");
+ f.setAccessible(true);
+ return (Set<UUID>) f.get(cache);
+ } catch (ReflectiveOperationException e) {
+ throw new IllegalStateException("Failed to access pendingEvictionQueue
via reflection", e);
+ }
+ }
+
+ private static IOzoneManagerLock newAcquiringLock() {
+ IOzoneManagerLock acquiringLock = mock(IOzoneManagerLock.class);
+ when(acquiringLock.acquireReadLock(eq(SNAPSHOT_DB_LOCK),
any(String[].class)))
+ .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED);
+ when(acquiringLock.releaseReadLock(eq(SNAPSHOT_DB_LOCK),
any(String[].class)))
+ .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED);
+ when(acquiringLock.acquireResourceWriteLock(eq(SNAPSHOT_DB_LOCK)))
+ .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED);
+ when(acquiringLock.releaseResourceWriteLock(eq(SNAPSHOT_DB_LOCK)))
+ .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED);
+ when(acquiringLock.acquireWriteLock(eq(SNAPSHOT_DB_LOCK),
any(String[].class)))
+ .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED);
+ when(acquiringLock.releaseWriteLock(eq(SNAPSHOT_DB_LOCK),
any(String[].class)))
+ .thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED);
Review Comment:
newAcquiringLock() stubs release* methods to return
EMPTY_DETAILS_LOCK_ACQUIRED, but the real OzoneManagerLock release paths clear
lockDetails and typically return isLockAcquired=false. Returning "acquired" on
release can make SnapshotCache.lock(...) believe the lock is still held after
an early unlock and attempt a second release on close, which can mask real
issues. Adjust these stubs so release* return EMPTY_DETAILS_LOCK_NOT_ACQUIRED
(and keep acquire* as ACQUIRED) to match production semantics.
```suggestion
.thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED);
when(acquiringLock.acquireResourceWriteLock(eq(SNAPSHOT_DB_LOCK)))
.thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED);
when(acquiringLock.releaseResourceWriteLock(eq(SNAPSHOT_DB_LOCK)))
.thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED);
when(acquiringLock.acquireWriteLock(eq(SNAPSHOT_DB_LOCK),
any(String[].class)))
.thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED);
when(acquiringLock.releaseWriteLock(eq(SNAPSHOT_DB_LOCK),
any(String[].class)))
.thenReturn(OMLockDetails.EMPTY_DETAILS_LOCK_NOT_ACQUIRED);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java:
##########
@@ -323,8 +324,13 @@ private UncheckedAutoCloseableSupplier<OMLockDetails>
lock(Supplier<OMLockDetail
AtomicReference<OMLockDetails> lockDetails = new
AtomicReference<>(emptyLockFunction.get());
if (lockDetails.get().isLockAcquired()) {
- if (!cleanupFunction.get()) {
+ try {
+ if (!cleanupFunction.get()) {
+ lockDetails.set(emptyUnlockFunction.get());
+ }
+ } catch (Throwable t) {
lockDetails.set(emptyUnlockFunction.get());
+ throw t;
}
Review Comment:
SnapshotCache.lock(...) can return a supplier whose OMLockDetails reports
lock not acquired when cleanupFunction returns false (eg, cache not drained).
The public lock() / lock(UUID) Javadocs currently describe the lock as ensuring
thread-safety, but don’t mention that callers must check isLockAcquired() (some
call sites use try-with-resources without checking). Consider either
documenting this contract explicitly here or throwing when cleanup can’t
satisfy the precondition so callers can’t proceed without the intended lock.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestSnapshotCache.java:
##########
@@ -511,4 +515,147 @@ void testSnapshotOperationsNotBlockedDuringCompaction()
throws IOException, Inte
verify(store1, times(1)).compactTable("table2");
verify(store1, times(0)).compactTable("keyTable");
}
+
+ @SuppressWarnings("unchecked")
+ private static Set<UUID> getPendingEvictionQueue(SnapshotCache cache) {
+ try {
+ Field f = SnapshotCache.class.getDeclaredField("pendingEvictionQueue");
+ f.setAccessible(true);
+ return (Set<UUID>) f.get(cache);
+ } catch (ReflectiveOperationException e) {
+ throw new IllegalStateException("Failed to access pendingEvictionQueue
via reflection", e);
+ }
Review Comment:
This test uses reflective access to SnapshotCache.pendingEvictionQueue. This
is brittle (field renames/module access can break it) and makes the test harder
to maintain. Prefer adding a `@VisibleForTesting` accessor on SnapshotCache
(eg, getPendingEvictionQueue()) and using that instead of setAccessible
reflection.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
