jojochuang commented on code in PR #3426:
URL: https://github.com/apache/ozone/pull/3426#discussion_r881054849
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/AbstractDatanodeStore.java:
##########
@@ -189,6 +189,19 @@ public BlockIterator<BlockData>
getBlockIterator(KeyPrefixFilter filter) {
blockDataTableWithIterator.iterator(), filter);
}
+ @Override
+ public boolean isClosed() {
Review Comment:
this.store is not thread-safe. It is better to make it volatile, and make
the stop() method synchronized.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java:
##########
@@ -280,7 +285,7 @@ private RocksDatabase(File dbFile, RocksDB db, DBOptions
dbOptions,
this.columnFamilies = columnFamilies;
}
- void close() {
+ public void close() {
Review Comment:
we should add a test to verify that when it is closed, isClosed() is true.
Same for RDBStore
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java:
##########
@@ -191,4 +191,41 @@ public void testConcurrentDBGet() throws Exception {
Assert.assertEquals(1, cache.size());
db.cleanup();
}
+
+ @Test
+ public void testUnderlyingDBzIsClosed() throws Exception {
+ File root = new File(testRoot);
+ root.mkdirs();
+
+ OzoneConfiguration conf = new OzoneConfiguration();
+ conf.setInt(OzoneConfigKeys.OZONE_CONTAINER_CACHE_SIZE, 2);
+
+ ContainerCache cache = ContainerCache.getInstance(conf);
+ cache.clear();
+ Assert.assertEquals(0, cache.size());
Review Comment:
it would be great to import org.junit.Assert.assertEquals and then just
write assertEquals().
I understand this is to follow the the existing code style in the file.
Just a very minor detail.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/AbstractDatanodeStore.java:
##########
@@ -189,6 +189,19 @@ public BlockIterator<BlockData>
getBlockIterator(KeyPrefixFilter filter) {
blockDataTableWithIterator.iterator(), filter);
}
+ @Override
+ public boolean isClosed() {
+ if (this.store == null) {
+ return true;
+ }
+ return this.store.isClosed();
+ }
+
+ @Override
+ public void close() throws IOException {
Review Comment:
does this one get used anywhere?
--
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]
