Copilot commented on code in PR #10218:
URL: https://github.com/apache/ozone/pull/10218#discussion_r3223643774
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -130,7 +130,12 @@ public OMRequest preExecute(OzoneManager ozoneManager)
throws IOException {
.setCreationTime(Time.now());
return
omRequest.toBuilder().setCreateSnapshotRequest(createSnapshotRequest.build()).build();
}
-
+
+ @Override
+ public void handleRequestFailure(OzoneManager ozoneManager) {
+ ozoneManager.getOmSnapshotManager().decrementInFlightSnapshotCount();
Review Comment:
This introduces new failure-path behavior affecting snapshot-limit
enforcement (decrementing the in-flight counter). Please add a unit/integration
test that exercises a post-`preExecute()` failure path (eg, PrepareState
rejection) and asserts the in-flight counter is decremented to prevent
unbounded growth.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -962,42 +962,45 @@ private void validateSnapshotsExistAndActive(final String
volumeName,
checkSnapshotActive(toSnapInfo, false);
}
+ /**
+ * Checks snapshot limit considering in-flight count and increments the
counter if allowed.
+ * Called in preExecute() to track and limit concurrent snapshot creations.
+ *
+ * @throws IOException if failed to get snapshot count
+ * @throws OMException if the snapshot limit would be exceeded
+ */
public void snapshotLimitCheck() throws IOException, OMException {
OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl)
ozoneManager.getMetadataManager();
SnapshotChainManager snapshotChainManager =
omMetadataManager.getSnapshotChainManager();
- AtomicReference<IOException> exceptionRef = new AtomicReference<>(null);
+ AtomicReference<Exception> exceptionRef = new AtomicReference<>(null);
inFlightSnapshotCount.updateAndGet(count -> {
- int currentSnapshotNum = 0;
+ int currentSnapshotCount;
try {
- currentSnapshotNum =
snapshotChainManager.getGlobalSnapshotChain().size();
+ currentSnapshotCount =
snapshotChainManager.getGlobalSnapshotChain().size();
+ throwIfSnapshotLimitExceeded(currentSnapshotCount, count);
} catch (IOException e) {
exceptionRef.set(e);
return count;
}
- if (currentSnapshotNum + count >= fsSnapshotMaxLimit) {
- exceptionRef.set(new OMException(
- String.format("Snapshot limit of %d reached. Cannot create more
snapshots. " +
- "Current snapshots: %d, In-flight creations: %d",
- fsSnapshotMaxLimit, currentSnapshotNum, count) +
- " If you already deleted some snapshots, " +
- "please wait for the background service to complete the
cleanup.",
- OMException.ResultCodes.TOO_MANY_SNAPSHOTS));
- return count;
- }
return count + 1;
});
if (exceptionRef.get() != null) {
- throw exceptionRef.get();
+ Exception e = exceptionRef.get();
+ if (e instanceof OMException) {
+ throw (OMException) e;
+ }
+ throw (IOException) e;
}
Review Comment:
`exceptionRef` is widened to `Exception`, but this code only ever stores
`IOException` (including `OMException`, which is already an `IOException`).
This adds avoidable casts and a future risk of `ClassCastException` if a
non-`IOException` is ever stored. Prefer `AtomicReference<IOException>` and
directly `throw exceptionRef.get();` (no `instanceof`/casts needed).
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -168,6 +173,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager, Execut
throw new OMException("Snapshot already exists", FILE_ALREADY_EXISTS);
}
+ ozoneManager.getOmSnapshotManager().assertSnapshotLimitNotExceeded();
Review Comment:
This adds a new hard safety-net check that can reject requests during
`validateAndUpdateCache()`. Add a test that sets the snapshot count to
`fsSnapshotMaxLimit` (or mocks the chain size) and validates that snapshot
creation fails at this point with `TOO_MANY_SNAPSHOTS`, ensuring the safety net
works independently of in-flight tracking.
--
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]
