HDDS-10184. Fix ManagedStatistics not closed properly #6055

whbing · 2024-01-22T12:26:46Z

What changes were proposed in this pull request?

When config set:

   <property>
      <name>ozone.metastore.rocksdb.statistics</name>
      <value>ALL</value>
   </property>

ManagedStatistics not closed properly in OM and DN:

2024-01-22 18:34:22,137 [LeakDetector-ManagedRocksObject0] WARN org.apache.hadoop.hdds.utils.db.managed.ManagedRocksObjectUtils: ManagedStatistics is not closed properly
StackTrace for unclosed instance: org.apache.hadoop.hdds.utils.db.managed.ManagedStatistics.<init>(ManagedStatistics.java:30)
org.apache.hadoop.hdds.utils.db.DBStoreBuilder.getDefaultDBOptions(DBStoreBuilder.java:418)
org.apache.hadoop.hdds.utils.db.DBStoreBuilder.build(DBStoreBuilder.java:209)
org.apache.hadoop.ozone.om.OmMetadataManagerImpl.loadDB(OmMetadataManagerImpl.java:607)
org.apache.hadoop.ozone.om.OmMetadataManagerImpl.loadDB(OmMetadataManagerImpl.java:570)
org.apache.hadoop.ozone.om.OmMetadataManagerImpl.start(OmMetadataManagerImpl.java:560)
org.apache.hadoop.ozone.om.OmMetadataManagerImpl.<init>(OmMetadataManagerImpl.java:342)
org.apache.hadoop.ozone.om.OzoneManager.instantiateServices(OzoneManager.java:803)
org.apache.hadoop.ozone.om.OzoneManager.<init>(OzoneManager.java:683)
org.apache.hadoop.ozone.om.OzoneManager.createOm(OzoneManager.java:768)
org.apache.hadoop.ozone.om.OzoneManagerStarter$OMStarterHelper.start(OzoneManagerStarter.java:189)
org.apache.hadoop.ozone.om.OzoneManagerStarter.startOm(OzoneManagerStarter.java:86)

2024-01-22 19:51:22,414 [LeakDetector-ManagedRocksObject0] WARN org.apache.hadoop.hdds.utils.db.managed.ManagedRocksObjectUtils: ManagedStatistics is not closed properly
StackTrace for unclosed instance: org.apache.hadoop.hdds.utils.db.managed.ManagedStatistics.<init>(ManagedStatistics.java:30)
org.apache.hadoop.ozone.container.metadata.AbstractDatanodeStore.start(AbstractDatanodeStore.java:121)
org.apache.hadoop.ozone.container.metadata.AbstractDatanodeStore.<init>(AbstractDatanodeStore.java:99)
org.apache.hadoop.ozone.container.metadata.DatanodeStoreSchemaThreeImpl.<init>(DatanodeStoreSchemaThreeImpl.java:66)
org.apache.hadoop.ozone.container.keyvalue.helpers.BlockUtils.getUncachedDatanodeStore(BlockUtils.java:85)
org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil.initPerDiskDBStore(HddsVolumeUtil.java:74)
org.apache.hadoop.ozone.container.common.volume.HddsVolume.loadDbStore(HddsVolume.java:365)
org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil.loadVolume(HddsVolumeUtil.java:111)
org.apache.hadoop.ozone.container.common.utils.HddsVolumeUtil.lambda$loadAllHddsVolumeDbStore$0(HddsVolumeUtil.java:97)
java.util.concurrent.CompletableFuture$AsyncRun.run(CompletableFuture.java:1626)
java.util.concurrent.CompletableFuture$AsyncRun.exec(CompletableFuture.java:1618)
java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10184

How was this patch tested?

origin exist test
start om and dn in test cluster, not reproduce after fixed

whbing · 2024-01-22T12:33:07Z

@adoroszlai PTAL, thanks you !

adoroszlai

Thanks @whbing for reporting this issue and working on the fix.

I wonder if Statistics should be kept open until DBOptions is closed.

sumitagrawl

@whbing Thanks working over this
ManagedStatistics is assigned to options, so do options close implicitly on its closer?
Even if we set ManagedStatistics to ManagedOption for closer, will there be twice close or not - One by rocksdb and another by ManagedOption(if its added).

So need to check do really need have leak detection for ManagedStatistics?

adoroszlai · 2024-01-23T09:45:45Z

ManagedStatistics is assigned to options, so do options close implicitly on its closer?

@sumitagrawl each object needs to be closed explicitly

https://github.com/facebook/rocksdb/blob/ef342246dc63c54d61909a6a5a0917769d83688a/java/samples/src/main/java/RocksDBSample.java#L30-L35

whbing · 2024-01-23T10:00:26Z

  private static GenericTestUtils.LogCapturer log = 
      GenericTestUtils.LogCapturer.captureLogs(ManagedRocksObjectUtils.LOG);
  assertFalse(log.getOutput().contains("is not closed properly"));

Simple verification of the startup log, fixed after closing ManagedStatistics

aswinshakil

Though this fixes this issue, We are closing the ManagedStatistics object before collecting any stats. I wonder if it records any of the stats if the ManagedStatistics is closed here.

whbing · 2024-01-24T05:10:37Z

The gc has already begun to reclaim the ManagedStatistics object after init ManagedStatistics instance during we startup the OM. This indicates that the ManagedStatistics object is not continuously referenced throughout the runtime of OM. Perhaps we should determine exactly when its lifecycle ends.

Maybe we can trace the metric changes after close ManagedStatistics to verify if it records any of the stats.

adoroszlai · 2024-01-24T06:08:58Z

I tested this for a short period, RocksDB statistics looked OK in Prometheus.

cd hadoop-ozone/dist/target/ozone-1.5.0-SNAPSHOT/compose/ozone
export COMPOSE_FILE=docker-compose.yaml:monitoring.yaml
OZONE_DATANODES=3 ./run.sh -d
docker-compose exec -T scm ozone freon ockg -n10000 -t4
open http://localhost:9090

Still, closing Statistics doesn't seem right. I think it should be kept in RDBStore (passed via DBStoreBuilder), and closed in RDBStore#close.

whbing · 2024-01-24T10:10:03Z

I think it should be kept in RDBStore (passed via DBStoreBuilder), and closed in RDBStore#close.

👍 @adoroszlai Thanks for reminding. I'll add a new commit later

whbing · 2024-01-24T15:22:17Z

@adoroszlai PTAL again if you have time , Thanks !

adoroszlai

Thanks @whbing for updating the patch, LGTM. Two minor possible improvements noted. If you agree with them, only apply them if there are any other requests for changes. Otherwise they are fine to be included in future tasks.

adoroszlai · 2024-01-24T20:04:18Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStore.java

+    if (statistics != null) {
+      IOUtils.close(LOG, statistics);
+    }
    IOUtils.close(LOG, db);


nit: IOUtils.close accepts multiple objects to be closed, and handles null values, too.

Suggested change

if (statistics != null) {

IOUtils.close(LOG, statistics);

}

IOUtils.close(LOG, db);

IOUtils.close(LOG, db, statistics);

adoroszlai · 2024-01-24T20:12:15Z

hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/DBStoreBuilder.java

+    if (!rocksDbStat.equals(OZONE_METADATA_STORE_ROCKSDB_STATISTICS_OFF)) {
+      statistics = new ManagedStatistics();
+      statistics.setStatsLevel(StatsLevel.valueOf(rocksDbStat));
+      dbOptions.setStatistics(statistics);
+    }


nit: ManagedStatistics statistics could be a local variable in build().

I don't think it makes any difference functionally, but it would be easier to follow its lifecycle. With instance variable, the reader needs to consider what happens if the same builder is used to build multiple RDBStore instances.

The global variable statistics are easier to pass to the RDBStore In current method structure, , nothing else.

aswinshakil

Thanks @whbing for the patch. LGTM.

whbing · 2024-01-25T03:33:44Z

@adoroszlai @aswinshakil @sumitagrawl Thank you for the above review !

sumitagrawl

LGTM

adoroszlai · 2024-01-25T07:55:25Z

Thanks @whbing for the fix, @aswinshakil, @sumitagrawl for the review.

(cherry picked from commit 9018728)

HDDS-10184. Fix ManagedStatistics not closed properly

dd0e0fa

adoroszlai reviewed Jan 22, 2024

View reviewed changes

sumitagrawl reviewed Jan 23, 2024

View reviewed changes

change close Statistics scope

3764acd

aswinshakil reviewed Jan 23, 2024

View reviewed changes

Statistics closed in RDBStore#close

d051057

adoroszlai self-requested a review January 24, 2024 15:24

adoroszlai approved these changes Jan 24, 2024

View reviewed changes

adoroszlai requested review from duongkame and aswinshakil January 24, 2024 20:31

aswinshakil approved these changes Jan 25, 2024

View reviewed changes

sumitagrawl approved these changes Jan 25, 2024

View reviewed changes

adoroszlai merged commit 9018728 into apache:master Jan 25, 2024
35 checks passed

adoroszlai pushed a commit to adoroszlai/ozone that referenced this pull request Jan 25, 2024

HDDS-10184. Fix ManagedStatistics not closed properly (apache#6055)

674790f

(cherry picked from commit 9018728)

adoroszlai mentioned this pull request Jan 25, 2024

[DO NOT MERGE] Backport some fixes from master to ozone-1.4 #6096

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-10184. Fix ManagedStatistics not closed properly #6055

HDDS-10184. Fix ManagedStatistics not closed properly #6055

whbing commented Jan 22, 2024 •

edited

Loading

whbing commented Jan 22, 2024

adoroszlai left a comment

sumitagrawl left a comment

adoroszlai commented Jan 23, 2024

whbing commented Jan 23, 2024

aswinshakil left a comment

whbing commented Jan 24, 2024

adoroszlai commented Jan 24, 2024

whbing commented Jan 24, 2024

whbing commented Jan 24, 2024

adoroszlai left a comment

adoroszlai Jan 24, 2024

adoroszlai Jan 24, 2024

whbing Jan 25, 2024

aswinshakil left a comment

whbing commented Jan 25, 2024

sumitagrawl left a comment

adoroszlai commented Jan 25, 2024

HDDS-10184. Fix ManagedStatistics not closed properly #6055

HDDS-10184. Fix ManagedStatistics not closed properly #6055

Conversation

whbing commented Jan 22, 2024 • edited Loading

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

whbing commented Jan 22, 2024

adoroszlai left a comment

Choose a reason for hiding this comment

sumitagrawl left a comment

Choose a reason for hiding this comment

adoroszlai commented Jan 23, 2024

whbing commented Jan 23, 2024

aswinshakil left a comment

Choose a reason for hiding this comment

whbing commented Jan 24, 2024

adoroszlai commented Jan 24, 2024

whbing commented Jan 24, 2024

whbing commented Jan 24, 2024

adoroszlai left a comment

Choose a reason for hiding this comment

adoroszlai Jan 24, 2024

Choose a reason for hiding this comment

adoroszlai Jan 24, 2024

Choose a reason for hiding this comment

whbing Jan 25, 2024

Choose a reason for hiding this comment

aswinshakil left a comment

Choose a reason for hiding this comment

whbing commented Jan 25, 2024

sumitagrawl left a comment

Choose a reason for hiding this comment

adoroszlai commented Jan 25, 2024

whbing commented Jan 22, 2024 •

edited

Loading