hemantk-12 commented on code in PR #6135:
URL: https://github.com/apache/ozone/pull/6135#discussion_r1476741473
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -397,11 +397,32 @@ private static CodecRegistry
createCodecRegistryForSnapDiff() {
}
/**
- * Get snapshot instance LRU cache.
- * @return LoadingCache
+ * Get snapshot instance LRU cache size. Used in tests.
+ * @return cache size.
*/
- public SnapshotCache getSnapshotCache() {
- return snapshotCache;
+ @VisibleForTesting
+ public int getSnapshotCacheSize() {
+ return snapshotCache.size();
Review Comment:
nit: add null check here.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -603,31 +624,64 @@ public ReferenceCounted<IOmMetadataReader, SnapshotCache>
checkForSnapshot(
String[] keyParts = keyName.split(OM_KEY_PREFIX);
if (isSnapshotKey(keyParts)) {
String snapshotName = keyParts[1];
- if (snapshotName == null || snapshotName.isEmpty()) {
- // don't allow snapshot indicator without snapshot name
- throw new OMException(INVALID_KEY_NAME);
- }
- String snapshotTableKey = SnapshotInfo.getTableKey(volumeName,
- bucketName, snapshotName);
- // Block FS API reads when snapshot is not active.
- if (!skipActiveCheck) {
- checkSnapshotActive(ozoneManager, snapshotTableKey);
- }
-
- // Warn if actual cache size exceeds the soft limit already.
- if (snapshotCache.size() > softCacheSize) {
- LOG.warn("Snapshot cache size ({}) exceeds configured soft-limit
({}).",
- snapshotCache.size(), softCacheSize);
- }
-
- // retrieve the snapshot from the cache
- return snapshotCache.get(snapshotTableKey, skipActiveCheck);
+ return (ReferenceCounted<IOmMetadataReader, SnapshotCache>)
(ReferenceCounted<?, ?>)
+ getActiveSnapshot(volumeName, bucketName, snapshotName);
} else {
return ozoneManager.getOmMetadataReader();
}
}
+ public ReferenceCounted<OmSnapshot, SnapshotCache> getActiveSnapshot(
+ String volumeName,
+ String bucketName,
+ String snapshotName) throws IOException {
+ return getSnapshot(volumeName, bucketName, snapshotName, false);
+ }
+
+ public ReferenceCounted<OmSnapshot, SnapshotCache> getSnapshot(
+ String volumeName,
+ String bucketName,
+ String snapshotName) throws IOException {
+ return getSnapshot(volumeName, bucketName, snapshotName, true);
+ }
+
+ private ReferenceCounted<OmSnapshot, SnapshotCache> getSnapshot(
+ String volumeName,
+ String bucketName,
+ String snapshotName,
+ boolean skipActiveCheck) throws IOException {
+
+ if (snapshotName == null || snapshotName.isEmpty()) {
+ // don't allow snapshot indicator without snapshot name
+ throw new OMException(INVALID_KEY_NAME);
+ }
+
+ String snapshotTableKey = SnapshotInfo.getTableKey(volumeName,
+ bucketName, snapshotName);
+
+ return getSnapshot(snapshotTableKey, skipActiveCheck);
+ }
+
+ private ReferenceCounted<OmSnapshot, SnapshotCache> getSnapshot(
+ String snapshotTableKey,
+ boolean skipActiveCheck) throws IOException {
+
+ // Block FS API reads when snapshot is not active.
+ if (!skipActiveCheck) {
+ checkSnapshotActive(ozoneManager, snapshotTableKey);
+ }
+
+ // Warn if actual cache size exceeds the soft limit already.
+ if (snapshotCache.size() > softCacheSize) {
+ LOG.warn("Snapshot cache size ({}) exceeds configured soft-limit ({}).",
+ snapshotCache.size(), softCacheSize);
+ }
Review Comment:
nit: this should be inside `SnapshotCache`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -397,11 +397,32 @@ private static CodecRegistry
createCodecRegistryForSnapDiff() {
}
/**
- * Get snapshot instance LRU cache.
- * @return LoadingCache
+ * Get snapshot instance LRU cache size. Used in tests.
Review Comment:
nit: no need to add "Used in tests." in comment. `@VisibleForTesting` is
already there and should be sued rather than a comment.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java:
##########
@@ -147,10 +147,9 @@ private void markSSTFilteredFlagForSnapshot(String volume,
String bucket,
@Override
public BackgroundTaskResult call() throws Exception {
- Optional<SnapshotCache> snapshotCache = Optional.ofNullable(ozoneManager)
- .map(OzoneManager::getOmSnapshotManager)
- .map(OmSnapshotManager::getSnapshotCache);
- if (!snapshotCache.isPresent()) {
+ Optional<OmSnapshotManager> snapshotManager =
Optional.ofNullable(ozoneManager)
+ .map(OzoneManager::getOmSnapshotManager);
+ if (!snapshotManager.isPresent()) {
Review Comment:
It will be evaluated to false even if snapshot feature is disabled because
snapshotManager obj is not null in that case. We should check if snapshot
feature is enabled and has some snapshots.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -590,11 +611,11 @@ private static void
deleteKeysFromDelKeyTableInSnapshotScope(
}
// Get OmSnapshot if the keyName has ".snapshot" key indicator
+ @SuppressWarnings("unchecked")
public ReferenceCounted<IOmMetadataReader, SnapshotCache> checkForSnapshot(
Review Comment:
nit: should this be called `getActiveFsOrSnapshotMetadata`? Or something
along the lines.
`checkForSnapshot` feels like would return boolean response.
--
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]
