[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request #1147: HDDS-3892. Datanode initialization is too slow when there are thousan…

[GitBox <gi...@apache.org>]

adoroszlai commented on a change in pull request #1147:
URL: https://github.com/apache/hadoop-ozone/pull/1147#discussion_r450182959



##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCache.java
##########
@@ -117,28 +117,49 @@ public ReferenceCountedDB getDB(long containerID, String containerDBType,
       throws IOException {
     Preconditions.checkState(containerID >= 0,
         "Container ID cannot be negative.");
+    ReferenceCountedDB db;
     lock.lock();
     try {
-      ReferenceCountedDB db = (ReferenceCountedDB) this.get(containerDBPath);
-
-      if (db == null) {
-        MetadataStore metadataStore =
-            MetadataStoreBuilder.newBuilder()
-            .setDbFile(new File(containerDBPath))
-            .setCreateIfMissing(false)
-            .setConf(conf)
-            .setDBType(containerDBType)
-            .build();
-        db = new ReferenceCountedDB(metadataStore, containerDBPath);
-        this.put(containerDBPath, db);
+      db = (ReferenceCountedDB) this.get(containerDBPath);
+      if (db != null) {
+        db.incrementReference();
+        return db;
       }
-      // increment the reference before returning the object
-      db.incrementReference();
-      return db;
+    } finally {
+      lock.unlock();
+    }
+
+    try {
+      MetadataStore metadataStore =
+          MetadataStoreBuilder.newBuilder()
+              .setDbFile(new File(containerDBPath))
+              .setCreateIfMissing(false)
+              .setConf(conf)
+              .setDBType(containerDBType)
+              .build();

Review comment:
       > > I see one issue with this approach.
   > > If the database is already opened, and if we try to open again we will get this error.
   > > I think, with this change, we will throw an exception if we try to open the database again an already existing one.
   > > java.io.IOException: Failed init RocksDB, db path : /Users/bviswanadham/workspace/hadoop-ozone/hadoop-hdds/container-service/target/test-dir/xCkBnsLVrc/cont1, exception :org.rocksdb.RocksDBException lock : /Users/bviswanadham/workspace/hadoop-ozone/hadoop-hdds/container-service/target/test-dir/xCkBnsLVrc/cont1/LOCK: No locks available
   > 
   > @bharatviswa504 , I get your point. It's an issue, but not introduced by this patch. It's a currenlty existing issue and we need to carefully think about how to fix it with a new JIRA.
   
   @ChenSammi can you please clarify why you think it's an existing issue?  It seems to me that calling `build()` outside of the lock introduces this problem.
   
   If I understand correctly, the performance issue is due to using a single lock for all containers.  Instead of moving the expensive part outside of the lock, how about using a separate lock per `containerDBPath`?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org