smengcl commented on code in PR #4249:
URL: https://github.com/apache/ozone/pull/4249#discussion_r1113303598
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -94,6 +95,10 @@ public OmSnapshot load(@Nonnull String snapshotTableKey)
// see if the snapshot exists
snapshotInfo = getSnapshotInfo(snapshotTableKey);
+ boolean isSnapshotInCache =
+ ozoneManager.getMetadataManager().getSnapshotInfoTable()
+ .getCacheValue(new CacheKey<>(snapshotTableKey)) != null;
Review Comment:
Is this accounting for the case where the snapshot entry is a tombstone in
cache (`CacheValue` is `Optional.absent()` like
[here](https://github.com/apache/ozone/blob/ddbe71d3f90ae14ecf29f51c2265f1f9c3171668/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java#L158-L162)),
which could happen when the snapshot eventually deleted from the snapshotInfo
table? Doesn't look to be the case to me?
Therefore, IMO in addition to checking null, we would need to check whether
the value is `Optional.absent()`. If it is `Optional.absent()`,
`isSnapshotInCache` should still be `false` as we won't want to wait for a
checkpoint dir that is deleted. Or we would have to put checks before this
logic is even reached.
--
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]
