hemantk-12 commented on code in PR #4568:
URL: https://github.com/apache/ozone/pull/4568#discussion_r1178722627
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -241,6 +253,30 @@ public OmSnapshotManager(OzoneManager ozoneManager) {
this.snapshotDiffCleanupService.start();
}
+ /**
+ * Throws OMException FILE_NOT_FOUND if snapshot is not in active status.
+ * @param snapshotTableKey snapshot table key
+ */
+ public void checkSnapshotActive(String snapshotTableKey) throws IOException {
+ checkSnapshotActive(getSnapshotInfo(snapshotTableKey));
+ }
+
+ private void checkSnapshotActive(SnapshotInfo snapInfo) throws OMException {
+
+ if (!snapInfo.getSnapshotStatus().equals(SnapshotStatus.SNAPSHOT_ACTIVE)) {
Review Comment:
```suggestion
if (snapshotInfo.getSnapshotStatus() != SnapshotStatus.SNAPSHOT_ACTIVE) {
```
nit to avoid null check.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -558,10 +584,14 @@ public SnapshotDiffResponse getSnapshotDiffReport(final
String volume,
final String fsKey = SnapshotInfo.getTableKey(volume, bucket,
fromSnapshot);
final String tsKey = SnapshotInfo.getTableKey(volume, bucket, toSnapshot);
try {
+ // Block SnapDiff if either one of the snapshot is not active.
+ checkSnapshotActive(fsKey);
Review Comment:
We do this check in `verifySnapshotInfoForSnapDiff` at line number 574. May
be you can change `verifySnapshotInfoForSnapDiff`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -241,6 +253,30 @@ public OmSnapshotManager(OzoneManager ozoneManager) {
this.snapshotDiffCleanupService.start();
}
+ /**
+ * Throws OMException FILE_NOT_FOUND if snapshot is not in active status.
+ * @param snapshotTableKey snapshot table key
+ */
+ public void checkSnapshotActive(String snapshotTableKey) throws IOException {
Review Comment:
I think it would be better to move this to
[SnapshotUtils](https://github.com/apache/ozone/blob/475963125b4a03a79a25519fe91fab4e1d6b36dc/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotUtils.java#L36).
I don't like the way it is used in `SnapshotDiffManager`.
```
ozoneManager.getOmSnapshotManager().checkSnapshotActive(fsKey);
ozoneManager.getOmSnapshotManager().checkSnapshotActive(tsKey);
```
I just realized that we have two Utils classes SnapshotUtils (created by me
:facepalm:) and OmSnapshotUtils. We can merge them to one as a separate task.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java:
##########
@@ -260,6 +260,16 @@ public void close() throws IOException {
omMetadataManager.getStore().close();
}
+ @Override
+ protected void finalize() throws Throwable {
Review Comment:
It is not generally [advice to override
finalize](https://softwareengineering.stackexchange.com/questions/288715/is-overriding-object-finalize-really-bad).
I see you did it to close GCed objects but I don't think it is needed.
RemovalListener will run on `Collected: Entry was removed automatically because
its key or value was garbage-collected.`
Point#3 in
https://www.educative.io/answers/how-to-use-eviction-listeners-and-statistics-in-guava-cache
https://guava.dev/releases/19.0/api/docs/com/google/common/cache/RemovalCause.html#COLLECTED
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -292,7 +309,7 @@ public OmSnapshot load(@Nonnull String snapshotTableKey)
snapshotMetadataManager = new OmMetadataManagerImpl(conf,
snapshotInfo.getCheckpointDirName(), isSnapshotInCache);
} catch (IOException e) {
- LOG.error("Failed to retrieve snapshot: {}, {}", snapshotTableKey,
e);
+ LOG.error("Failed to retrieve snapshot: {}", snapshotTableKey, e);
Review Comment:
Curious if we are doing double logging. Won't it be thrown as
ExecutionException.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotFileSystem.java:
##########
@@ -411,15 +411,19 @@ public void testBlockSnapshotFSAccessAfterDeletion()
throws Exception {
deleteSnapshot(snapshotName);
// Can't access keys in snapshot anymore with FS API. Should throw
exception
- final String errorMsg = "no longer active";
- LambdaTestUtils.intercept(OMException.class, errorMsg,
+ final String errorMsg1 = "no longer active";
+ LambdaTestUtils.intercept(FileNotFoundException.class, errorMsg1,
Review Comment:
+1 on checking exception.
--
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]
