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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
