You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "hemantk-12 (via GitHub)" <gi...@apache.org> on 2023/05/10 20:30:04 UTC
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #4692: HDDS-8576. Close RocksDB instance in RDBStore if RDBStore's initialization fails after RocksDB instance creation
hemantk-12 commented on code in PR #4692:
URL: https://github.com/apache/ozone/pull/4692#discussion_r1190358519
##########
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) {
Review Comment:
Changed it to IOException and verified that test works.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -372,22 +372,28 @@ private OmMetadataManagerImpl(OzoneConfiguration conf, File dir, String name)
// metadata constructor for snapshots
OmMetadataManagerImpl(OzoneConfiguration conf, String snapshotDirName,
boolean isSnapshotInCache) throws IOException {
- lock = new OmReadOnlyLock();
- omEpoch = 0;
- String snapshotDir = OMStorage.getOmDbDir(conf) +
- OM_KEY_PREFIX + OM_SNAPSHOT_CHECKPOINT_DIR;
- File metaDir = new File(snapshotDir);
- String dbName = OM_DB_NAME + snapshotDirName;
- // The check is only to prevent every snapshot read to perform a disk IO
- // and check if a checkpoint dir exists. If entry is present in cache,
- // it is most likely DB entries will get flushed in this wait time.
- if (isSnapshotInCache) {
- File checkpoint = Paths.get(metaDir.toPath().toString(), dbName).toFile();
- RDBCheckpointUtils.waitForCheckpointDirectoryExist(checkpoint);
+ try {
+ lock = new OmReadOnlyLock();
+ omEpoch = 0;
+ String snapshotDir = OMStorage.getOmDbDir(conf) +
+ OM_KEY_PREFIX + OM_SNAPSHOT_CHECKPOINT_DIR;
+ File metaDir = new File(snapshotDir);
+ String dbName = OM_DB_NAME + snapshotDirName;
+ // The check is only to prevent every snapshot read to perform a disk IO
+ // and check if a checkpoint dir exists. If entry is present in cache,
+ // it is most likely DB entries will get flushed in this wait time.
+ if (isSnapshotInCache) {
+ File checkpoint =
+ Paths.get(metaDir.toPath().toString(), dbName).toFile();
+ RDBCheckpointUtils.waitForCheckpointDirectoryExist(checkpoint);
+ }
+ setStore(loadDB(conf, metaDir, dbName, false,
+ java.util.Optional.of(Boolean.TRUE), false));
+ initializeOmTables(false);
+ } catch (Exception e) {
+ stop();
+ throw new IOException(e.getCause());
Review Comment:
Changed the catch to IOException.
##########
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:
It is not advised to override the finalized method to close object. For more check: https://softwareengineering.stackexchange.com/questions/288715/is-overriding-object-finalize-really-bad/288724#288724
I added a WARN log statement instead of this to catch any memory leak.
##########
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:
`Optional` is a great way to avoid null check when passing the object to or receiving from another method. And can be used as class variable if object is supposed to be nullable. But I don't think it is a good idea for this type of case when object is supposed to be not null and it is null because initialized failed or this case.
In this current scenario, `snapshotMetadataManager` should not be null at class's object level and if it is then there is some problem which needs investigation.
So I prefer to not use `Optional` in this case.
##########
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 &&
+ !snapshotMetadataManager.getStore().isClosed()) {
+ snapshotMetadataManager.getStore().close();
+ }
+ throw new IOException(e.getCause());
Review Comment:
Same as above.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org