swamirishi commented on code in PR #4692:
URL: https://github.com/apache/ozone/pull/4692#discussion_r1190338480
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -295,47 +295,57 @@ private CacheLoader<String, OmSnapshot>
createCacheLoader() {
// load the snapshot into the cache if not already there
@Nonnull
public OmSnapshot load(@Nonnull String snapshotTableKey)
- throws IOException {
- // see if the snapshot exists
- SnapshotInfo snapshotInfo = getSnapshotInfo(snapshotTableKey);
-
- // Block snapshot from loading when it is no longer active e.g.
DELETED,
- // unless this is called from SnapshotDeletingService.
- checkSnapshotActive(snapshotInfo);
-
- CacheValue<SnapshotInfo> cacheValue =
- ozoneManager.getMetadataManager().getSnapshotInfoTable()
- .getCacheValue(new CacheKey<>(snapshotTableKey));
- boolean isSnapshotInCache = cacheValue != null && Optional.ofNullable(
- cacheValue.getCacheValue()).isPresent();
-
- // read in the snapshot
- OzoneConfiguration conf = ozoneManager.getConfiguration();
- OMMetadataManager snapshotMetadataManager;
-
- // Create the snapshot metadata manager by finding the corresponding
- // RocksDB instance, creating an OmMetadataManagerImpl instance based
on
- // that
+ throws Exception {
+ OMMetadataManager snapshotMetadataManager = null;
try {
- snapshotMetadataManager = new OmMetadataManagerImpl(conf,
- snapshotInfo.getCheckpointDirName(), isSnapshotInCache);
- } catch (IOException e) {
- LOG.error("Failed to retrieve snapshot: {}", snapshotTableKey);
- throw e;
+ // see if the snapshot exists
+ SnapshotInfo snapshotInfo = getSnapshotInfo(snapshotTableKey);
+
+ // Block snapshot from loading when it is no longer active
+ // e.g. DELETED, unless this is called from SnapshotDeletingService.
+ checkSnapshotActive(snapshotInfo);
+
+ CacheValue<SnapshotInfo> cacheValue =
+ ozoneManager.getMetadataManager().getSnapshotInfoTable()
+ .getCacheValue(new CacheKey<>(snapshotTableKey));
+ boolean isSnapshotInCache = cacheValue != null &&
Optional.ofNullable(
+ cacheValue.getCacheValue()).isPresent();
+
+ // read in the snapshot
+ OzoneConfiguration conf = ozoneManager.getConfiguration();
+
+ // Create the snapshot metadata manager by finding the corresponding
+ // RocksDB instance, creating an OmMetadataManagerImpl instance based
+ // on that.
+ try {
+ snapshotMetadataManager = new OmMetadataManagerImpl(conf,
+ snapshotInfo.getCheckpointDirName(), isSnapshotInCache);
+ } catch (IOException e) {
+ LOG.error("Failed to retrieve snapshot: {}", snapshotTableKey);
+ throw e;
+ }
+
+ // create the other manager instances based on snapshot
+ // metadataManager
+ PrefixManagerImpl pm = new PrefixManagerImpl(snapshotMetadataManager,
+ false);
+ KeyManagerImpl km = new KeyManagerImpl(null,
+ ozoneManager.getScmClient(), snapshotMetadataManager, conf,
+ ozoneManager.getBlockTokenSecretManager(),
+ ozoneManager.getKmsProvider(), ozoneManager.getPerfMetrics());
+
+ return new OmSnapshot(km, pm, ozoneManager,
+ snapshotInfo.getVolumeName(),
+ snapshotInfo.getBucketName(),
+ snapshotInfo.getName());
+ } catch (Exception e) {
+ // Close RocksDB if snapshotMetadataManager got initialized.
+ if (snapshotMetadataManager != null &&
Review Comment:
Nit: I usually don't like null checks, prefer to use optional instead when I
know objects can have null value.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java:
##########
@@ -73,7 +73,7 @@
* When there is a {@link RocksDBException} with error,
* this class will close the underlying {@link org.rocksdb.RocksObject}s.
*/
-public final class RocksDatabase {
+public final class RocksDatabase implements Closeable {
Review Comment:
Can we add the close in the finalize method of this object as a fail safe, &
log a warning if it is not being closed properly with the stacktrace of
creation of the object. Would this of any help?
--
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]
