From 7918215b2daf823981a8db6ce1e45aa2ef2da5d0 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Tue, 3 Nov 2020 12:57:10 +0000 Subject: [PATCH 1/6] Avoid ContainerCache when starting the DN --- .../common/utils/ContainerCache.java | 19 +---- .../keyvalue/helpers/BlockUtils.java | 54 +++++++++++++ .../helpers/KeyValueContainerUtil.java | 78 +++++++++---------- .../metadata/AbstractDatanodeStore.java | 5 ++ .../container/metadata/DatanodeStore.java | 2 +- .../ozoneimpl/TestContainerReader.java | 13 +++- 6 files changed, 109 insertions(+), 62 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCache.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCache.java index 506fb31f0cb7..02bd5ce44d29 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCache.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCache.java @@ -30,10 +30,8 @@ import com.google.common.base.Preconditions; import org.apache.commons.collections.MapIterator; import org.apache.commons.collections.map.LRUMap; -import org.apache.hadoop.ozone.OzoneConsts; +import org.apache.hadoop.ozone.container.keyvalue.helpers.BlockUtils; import org.apache.hadoop.ozone.container.metadata.DatanodeStore; -import org.apache.hadoop.ozone.container.metadata.DatanodeStoreSchemaOneImpl; -import org.apache.hadoop.ozone.container.metadata.DatanodeStoreSchemaTwoImpl; import org.apache.hadoop.util.Time; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -158,19 +156,8 @@ public ReferenceCountedDB getDB(long containerID, String containerDBType, try { long start = Time.monotonicNow(); - DatanodeStore store; - - if (schemaVersion.equals(OzoneConsts.SCHEMA_V1)) { - store = new DatanodeStoreSchemaOneImpl(conf, - containerID, containerDBPath); - } else if (schemaVersion.equals(OzoneConsts.SCHEMA_V2)) { - store = new DatanodeStoreSchemaTwoImpl(conf, - containerID, containerDBPath); - } else { - throw new IllegalArgumentException( - "Unrecognized database schema version: " + schemaVersion); - } - + DatanodeStore store = BlockUtils.getUncachedDatanodeStore(containerID, + schemaVersion, containerDBPath, conf); db = new ReferenceCountedDB(store, containerDBPath); metrics.incDbOpenLatency(Time.monotonicNow() - start); } catch (Exception e) { diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/BlockUtils.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/BlockUtils.java index caff36c05709..0a8d692afd95 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/BlockUtils.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/BlockUtils.java @@ -23,12 +23,17 @@ import org.apache.hadoop.hdds.conf.ConfigurationSource; import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos; import org.apache.hadoop.hdds.scm.container.common.helpers.StorageContainerException; +import org.apache.hadoop.ozone.OzoneConsts; import org.apache.hadoop.ozone.container.common.helpers.BlockData; import org.apache.hadoop.ozone.container.common.utils.ContainerCache; import org.apache.hadoop.ozone.container.common.utils.ReferenceCountedDB; import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData; import com.google.common.base.Preconditions; +import org.apache.hadoop.ozone.container.metadata.DatanodeStore; +import org.apache.hadoop.ozone.container.metadata.DatanodeStoreSchemaOneImpl; +import org.apache.hadoop.ozone.container.metadata.DatanodeStoreSchemaTwoImpl; + import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.Result.NO_SUCH_BLOCK; import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.Result.UNABLE_TO_READ_METADATA_DB; @@ -41,6 +46,55 @@ public final class BlockUtils { private BlockUtils() { } + + /** + * Obtain a DB handler for a given container. This handler is not cached and + * the caller must close it after using it. + * If another thread attempts to open the same container when it is already + * opened by this thread, the other thread will get a RocksDB exception. + * @param containerID The containerID + * @param containerDBPath The absolute path to the container database folder + * @param schemaVersion The Container Schema version + * @param conf Configuration + * @return Handler to the given container. + * @throws IOException + */ + public static DatanodeStore getUncachedDatanodeStore(long containerID, + String containerDBPath, String schemaVersion, + ConfigurationSource conf) throws IOException { + + DatanodeStore store; + if (schemaVersion.equals(OzoneConsts.SCHEMA_V1)) { + store = new DatanodeStoreSchemaOneImpl(conf, + containerID, containerDBPath); + } else if (schemaVersion.equals(OzoneConsts.SCHEMA_V2)) { + store = new DatanodeStoreSchemaTwoImpl(conf, + containerID, containerDBPath); + } else { + throw new IllegalArgumentException( + "Unrecognized database schema version: " + schemaVersion); + } + return store; + } + + /** + * Obtain a DB handler for a given container. This handler is not cached and + * the caller must close it after using it. + * If another thread attempts to open the same container when it is already + * opened by this thread, the other thread will get a RocksDB exception. + * @param containerData The container data + * @param conf Configuration + * @return + * @throws IOException + */ + public static DatanodeStore getUncachedDatanodeStore( + KeyValueContainerData containerData, ConfigurationSource conf) + throws IOException { + return getUncachedDatanodeStore(containerData.getContainerID(), + containerData.getDbFile().getAbsolutePath(), + containerData.getSchemaVersion(), conf); + } + /** * Get a DB handler for a given container. * If the handler doesn't exist in cache yet, first create one and diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java index e807c9b9c880..dafd999035b8 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java @@ -190,11 +190,9 @@ public static void parseKVContainerData(KeyValueContainerData kvContainerData, boolean isBlockMetadataSet = false; - try(ReferenceCountedDB containerDB = BlockUtils.getDB(kvContainerData, - config)) { - - Table metadataTable = - containerDB.getStore().getMetadataTable(); + try(DatanodeStore store = + BlockUtils.getUncachedDatanodeStore(kvContainerData, config)) { + Table metadataTable = store.getMetadataTable(); // Set pending deleted block count. Long pendingDeleteBlockCount = @@ -207,7 +205,7 @@ public static void parseKVContainerData(KeyValueContainerData kvContainerData, MetadataKeyFilters.KeyPrefixFilter filter = MetadataKeyFilters.getDeletingKeyFilter(); int numPendingDeletionBlocks = - containerDB.getStore().getBlockDataTable() + store.getBlockDataTable() .getSequentialRangeKVs(null, Integer.MAX_VALUE, filter) .size(); kvContainerData.incrPendingDeletionBlocks(numPendingDeletionBlocks); @@ -244,61 +242,55 @@ public static void parseKVContainerData(KeyValueContainerData kvContainerData, isBlockMetadataSet = true; kvContainerData.setKeyCount(blockCount); } - } - - if (!isBlockMetadataSet) { - initializeUsedBytesAndBlockCount(kvContainerData, config); + if (!isBlockMetadataSet) { + initializeUsedBytesAndBlockCount(store, kvContainerData); + } + } catch (Exception e) { + LOG.error("Unexpected exception handling container {}", containerID, e); + throw new IOException(e); } } - /** * Initialize bytes used and block count. * @param kvData * @throws IOException */ - private static void initializeUsedBytesAndBlockCount( - KeyValueContainerData kvData, ConfigurationSource config) - throws IOException { - + private static void initializeUsedBytesAndBlockCount(DatanodeStore store, + KeyValueContainerData kvData) throws IOException { final String errorMessage = "Failed to parse block data for" + - " Container " + kvData.getContainerID(); - + " Container " + kvData.getContainerID(); long blockCount = 0; long usedBytes = 0; - try(ReferenceCountedDB db = BlockUtils.getDB(kvData, config)) { - // Count all regular blocks. - try (BlockIterator blockIter = - db.getStore().getBlockIterator( - MetadataKeyFilters.getUnprefixedKeyFilter())) { - - while (blockIter.hasNext()) { - blockCount++; - try { - usedBytes += getBlockLength(blockIter.nextBlock()); - } catch (IOException ex) { - LOG.error(errorMessage); - } + try (BlockIterator blockIter = + store.getBlockIterator( + MetadataKeyFilters.getUnprefixedKeyFilter())) { + + while (blockIter.hasNext()) { + blockCount++; + try { + usedBytes += getBlockLength(blockIter.nextBlock()); + } catch (IOException ex) { + LOG.error(errorMessage); } } + } - // Count all deleting blocks. - try (BlockIterator blockIter = - db.getStore().getBlockIterator( - MetadataKeyFilters.getDeletingKeyFilter())) { - - while (blockIter.hasNext()) { - blockCount++; - try { - usedBytes += getBlockLength(blockIter.nextBlock()); - } catch (IOException ex) { - LOG.error(errorMessage); - } + // Count all deleting blocks. + try (BlockIterator blockIter = + store.getBlockIterator( + MetadataKeyFilters.getDeletingKeyFilter())) { + + while (blockIter.hasNext()) { + blockCount++; + try { + usedBytes += getBlockLength(blockIter.nextBlock()); + } catch (IOException ex) { + LOG.error(errorMessage); } } } - kvData.setBytesUsed(usedBytes); kvData.setKeyCount(blockCount); } diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/AbstractDatanodeStore.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/AbstractDatanodeStore.java index 6c258eda7cd0..62df69bbb281 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/AbstractDatanodeStore.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/AbstractDatanodeStore.java @@ -127,6 +127,11 @@ public void stop() throws Exception { } } + @Override + public void close() throws Exception { + stop(); + } + @Override public DBStore getStore() { return this.store; diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStore.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStore.java index 5a0ce7a64623..c6af57b5f293 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStore.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStore.java @@ -32,7 +32,7 @@ /** * Interface for interacting with datanode databases. */ -public interface DatanodeStore { +public interface DatanodeStore extends AutoCloseable { /** * Start datanode manager. diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java index 07f1d485b048..7855b692dde8 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java @@ -112,7 +112,10 @@ public void setup() throws Exception { blkNames = addBlocks(keyValueContainer, false); markBlocksForDelete(keyValueContainer, false, blkNames, i); } - + // Close the RocksDB instance for this container and remove from the cache + // so it does not affect the ContainerReader, which avoids using the cache + // at startup for performance reasons. + BlockUtils.removeDB(keyValueContainerData, conf); } } @@ -250,6 +253,10 @@ public void testMultipleContainerReader() throws Exception { blkNames = addBlocks(keyValueContainer, false); markBlocksForDelete(keyValueContainer, false, blkNames, i); } + // Close the RocksDB instance for this container and remove from the cache + // so it does not affect the ContainerReader, which avoids using the cache + // at startup for performance reasons. + BlockUtils.removeDB(keyValueContainerData, conf); } List hddsVolumes = volumeSets.getVolumesList(); @@ -271,6 +278,8 @@ public void testMultipleContainerReader() throws Exception { " costs " + (System.currentTimeMillis() - startTime) / 1000 + "s"); Assert.assertEquals(containerCount, containerSet.getContainerMap().entrySet().size()); - Assert.assertEquals(containerCount, cache.size()); + // There should be no open containers cached by the ContainerReader as it + // opens and closed them avoiding the cache. + Assert.assertEquals(0, cache.size()); } } From 725fafd1a4c29eeb1889bd665ce1adbf908a61f5 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Tue, 3 Nov 2020 20:26:51 +0000 Subject: [PATCH 2/6] Fix failing tests --- .../common/utils/ContainerCache.java | 2 +- .../TestSchemaOneBackwardsCompatibility.java | 35 +++++++++++-------- .../ozoneimpl/TestOzoneContainer.java | 2 ++ 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCache.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCache.java index 02bd5ce44d29..a7fa54a1797f 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCache.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerCache.java @@ -157,7 +157,7 @@ public ReferenceCountedDB getDB(long containerID, String containerDBType, try { long start = Time.monotonicNow(); DatanodeStore store = BlockUtils.getUncachedDatanodeStore(containerID, - schemaVersion, containerDBPath, conf); + containerDBPath, schemaVersion, conf); db = new ReferenceCountedDB(store, containerDBPath); metrics.incDbOpenLatency(Time.monotonicNow() - start); } catch (Exception e) { diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestSchemaOneBackwardsCompatibility.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestSchemaOneBackwardsCompatibility.java index 01fa3bf372c4..99e8ea97ea0b 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestSchemaOneBackwardsCompatibility.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestSchemaOneBackwardsCompatibility.java @@ -31,6 +31,7 @@ import org.apache.hadoop.ozone.container.common.impl.ContainerSet; import org.apache.hadoop.ozone.container.common.interfaces.BlockIterator; import org.apache.hadoop.ozone.container.common.interfaces.ContainerDispatcher; +import org.apache.hadoop.ozone.container.common.utils.ContainerCache; import org.apache.hadoop.ozone.container.common.utils.ReferenceCountedDB; import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainer; import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData; @@ -197,7 +198,8 @@ public void testReadWithMetadata() throws Exception { public void testReadWithoutMetadata() throws Exception { // Delete metadata keys from our copy of the DB. // This simulates them not being there to start with. - try (ReferenceCountedDB db = BlockUtils.getDB(newKvData(), conf)) { + KeyValueContainerData kvData = newKvData(); + try (ReferenceCountedDB db = BlockUtils.getDB(kvData, conf)) { Table metadataTable = db.getStore().getMetadataTable(); metadataTable.delete(OzoneConsts.BLOCK_COUNT); @@ -237,7 +239,8 @@ public void testDelete() throws Exception { final long expectedRegularBlocks = TestDB.KEY_COUNT - numBlocksToDelete; - try(ReferenceCountedDB refCountedDB = BlockUtils.getDB(newKvData(), conf)) { + KeyValueContainerData kvData = newKvData(); + try(ReferenceCountedDB refCountedDB = BlockUtils.getDB(kvData, conf)) { // Test results via block iteration. assertEquals(expectedDeletingBlocks, countDeletingBlocks(refCountedDB)); @@ -269,29 +272,29 @@ public void testDelete() throws Exception { */ @Test public void testReadDeletedBlockChunkInfo() throws Exception { - try(ReferenceCountedDB refCountedDB = BlockUtils.getDB(newKvData(), conf)) { + KeyValueContainerData kvData = newKvData(); + List> deletedBlocks; + Set preUpgradeBlocks = new HashSet<>(); + try(ReferenceCountedDB refCountedDB = BlockUtils.getDB(kvData, conf)) { // Read blocks that were already deleted before the upgrade. - List> deletedBlocks = - refCountedDB.getStore() - .getDeletedBlocksTable().getRangeKVs(null, 100); - - Set preUpgradeBlocks = new HashSet<>(); + deletedBlocks = refCountedDB.getStore() + .getDeletedBlocksTable().getRangeKVs(null, 100); - for(Table.KeyValue chunkListKV: deletedBlocks) { + for (Table.KeyValue chunkListKV : deletedBlocks) { preUpgradeBlocks.add(chunkListKV.getKey()); try { chunkListKV.getValue(); Assert.fail("No exception thrown when trying to retrieve old " + - "deleted blocks values as chunk lists."); - } catch(IOException ex) { + "deleted blocks values as chunk lists."); + } catch (IOException ex) { // Exception thrown as expected. } } + } - Assert.assertEquals(TestDB.NUM_DELETED_BLOCKS, preUpgradeBlocks.size()); - - runBlockDeletingService(); + runBlockDeletingService(); + try(ReferenceCountedDB refCountedDB = BlockUtils.getDB(kvData, conf)) { // After the block deleting service runs, get the updated list of // deleted blocks. deletedBlocks = refCountedDB.getStore() @@ -494,6 +497,10 @@ private OzoneContainer makeMockOzoneContainer() throws Exception { * @throws IOException */ private KeyValueContainerData newKvData() throws IOException { + // The parseKVContainerData() method opens RocksDB without using the + // container cache, so we need to ensure the DB is not already open + // by purging the cache before opening it. + BlockUtils.shutdownCache(ContainerCache.getInstance(conf)); KeyValueContainerData kvData = (KeyValueContainerData) ContainerDataYaml.readContainerFile(containerFile); diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainer.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainer.java index bc4bc259bafd..982cea35f830 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainer.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainer.java @@ -148,6 +148,7 @@ public void testBuildContainerMap() throws Exception { Preconditions.checkState(freeBytes >= 0); commitSpaceMap.put(getVolumeKey(myVolume), Long.valueOf(volCommitBytes + freeBytes)); + BlockUtils.removeDB(keyValueContainerData, conf); } DatanodeStateMachine stateMachine = Mockito.mock( @@ -290,6 +291,7 @@ private long addBlocks(KeyValueContainer container, metadataTable.put(OzoneConsts.CONTAINER_BYTES_USED, usedBytes); // remaining available capacity of the container + db.close(); return (freeBytes - usedBytes); } From 45cfc53adf92e2385e250f8aa60959061469e0d5 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Tue, 3 Nov 2020 21:05:46 +0000 Subject: [PATCH 3/6] Fix findbugs warning --- .../container/keyvalue/helpers/KeyValueContainerUtil.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java index dafd999035b8..32e02b514fe1 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java @@ -245,9 +245,13 @@ public static void parseKVContainerData(KeyValueContainerData kvContainerData, if (!isBlockMetadataSet) { initializeUsedBytesAndBlockCount(store, kvContainerData); } + } catch (IOException e) { + throw e; } catch (Exception e) { - LOG.error("Unexpected exception handling container {}", containerID, e); - throw new IOException(e); + LOG.error("Unexpected error closing the DatanodeStore for container {}", + containerID, e); + throw new RuntimeException("Unexpected error closing the DatanodeStore " + + "for container "+containerID, e); } } From 0e23ee2c63f57dceed8e2b58b32dea3f6ba72d99 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Mon, 9 Nov 2020 12:22:57 +0000 Subject: [PATCH 4/6] Refactor to obtain the RocksDB instance from the ContainerCache if the uncached instance fails to open --- .../helpers/KeyValueContainerUtil.java | 43 ++++++++++++++----- .../metadata/AbstractDatanodeStore.java | 5 --- .../container/metadata/DatanodeStore.java | 4 +- .../TestSchemaOneBackwardsCompatibility.java | 4 +- 4 files changed, 36 insertions(+), 20 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java index 32e02b514fe1..1780b1ebf0e3 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java @@ -187,11 +187,22 @@ public static void parseKVContainerData(KeyValueContainerData kvContainerData, kvContainerData.setSchemaVersion(OzoneConsts.SCHEMA_V1); } - boolean isBlockMetadataSet = false; - - try(DatanodeStore store = - BlockUtils.getUncachedDatanodeStore(kvContainerData, config)) { + ReferenceCountedDB cachedDB = null; + DatanodeStore store = null; + try { + try { + store = BlockUtils.getUncachedDatanodeStore(kvContainerData, config); + } catch (IOException e) { + // If an exception is thrown, then it may indicate the RocksDB is + // already open in the container cache. As this code is only executed at + // DN startup, this should only happen in the tests. + cachedDB = BlockUtils.getDB(kvContainerData, config); + store = cachedDB.getStore(); + LOG.warn("Attempt to get an uncached RocksDB handle failed and an " + + "instance was retrieved from the cache. This should only happen " + + "in tests"); + } Table metadataTable = store.getMetadataTable(); // Set pending deleted block count. @@ -245,13 +256,23 @@ public static void parseKVContainerData(KeyValueContainerData kvContainerData, if (!isBlockMetadataSet) { initializeUsedBytesAndBlockCount(store, kvContainerData); } - } catch (IOException e) { - throw e; - } catch (Exception e) { - LOG.error("Unexpected error closing the DatanodeStore for container {}", - containerID, e); - throw new RuntimeException("Unexpected error closing the DatanodeStore " + - "for container "+containerID, e); + } finally { + if (cachedDB != null) { + // If we get a cached instance, calling close simply decrements the + // reference count. + cachedDB.close(); + } else if (store != null) { + // We only stop the store if cacheDB is null, as otherwise we would + // close the rocksDB handle in the cache and the next reader would fail + try { + store.stop(); + } catch (IOException e) { + throw e; + } catch (Exception e) { + throw new RuntimeException("Unexpected exception closing the " + + "RocksDB when loading containers", e); + } + } } } diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/AbstractDatanodeStore.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/AbstractDatanodeStore.java index 62df69bbb281..6c258eda7cd0 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/AbstractDatanodeStore.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/AbstractDatanodeStore.java @@ -127,11 +127,6 @@ public void stop() throws Exception { } } - @Override - public void close() throws Exception { - stop(); - } - @Override public DBStore getStore() { return this.store; diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStore.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStore.java index c6af57b5f293..992bce70e3a1 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStore.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStore.java @@ -32,7 +32,7 @@ /** * Interface for interacting with datanode databases. */ -public interface DatanodeStore extends AutoCloseable { +public interface DatanodeStore { /** * Start datanode manager. @@ -47,7 +47,7 @@ public interface DatanodeStore extends AutoCloseable { */ void stop() throws Exception; - /** + /**TestSchemaOneBackwardsCompatibility.java * Get datanode store. * * @return datanode store. diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestSchemaOneBackwardsCompatibility.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestSchemaOneBackwardsCompatibility.java index 99e8ea97ea0b..0a084ee6eb36 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestSchemaOneBackwardsCompatibility.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestSchemaOneBackwardsCompatibility.java @@ -277,8 +277,8 @@ public void testReadDeletedBlockChunkInfo() throws Exception { Set preUpgradeBlocks = new HashSet<>(); try(ReferenceCountedDB refCountedDB = BlockUtils.getDB(kvData, conf)) { // Read blocks that were already deleted before the upgrade. - deletedBlocks = refCountedDB.getStore() - .getDeletedBlocksTable().getRangeKVs(null, 100); + deletedBlocks = refCountedDB.getStore() + .getDeletedBlocksTable().getRangeKVs(null, 100); for (Table.KeyValue chunkListKV : deletedBlocks) { preUpgradeBlocks.add(chunkListKV.getKey()); From 73f8f6356cda50b123bbd9d040bc1f18a1b5ccd6 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Mon, 9 Nov 2020 16:01:48 +0000 Subject: [PATCH 5/6] Revert changes to TestSchemaOneBackwardsCompatibility.java --- .../TestSchemaOneBackwardsCompatibility.java | 35 ++++++++----------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestSchemaOneBackwardsCompatibility.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestSchemaOneBackwardsCompatibility.java index 0a084ee6eb36..01fa3bf372c4 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestSchemaOneBackwardsCompatibility.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestSchemaOneBackwardsCompatibility.java @@ -31,7 +31,6 @@ import org.apache.hadoop.ozone.container.common.impl.ContainerSet; import org.apache.hadoop.ozone.container.common.interfaces.BlockIterator; import org.apache.hadoop.ozone.container.common.interfaces.ContainerDispatcher; -import org.apache.hadoop.ozone.container.common.utils.ContainerCache; import org.apache.hadoop.ozone.container.common.utils.ReferenceCountedDB; import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainer; import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData; @@ -198,8 +197,7 @@ public void testReadWithMetadata() throws Exception { public void testReadWithoutMetadata() throws Exception { // Delete metadata keys from our copy of the DB. // This simulates them not being there to start with. - KeyValueContainerData kvData = newKvData(); - try (ReferenceCountedDB db = BlockUtils.getDB(kvData, conf)) { + try (ReferenceCountedDB db = BlockUtils.getDB(newKvData(), conf)) { Table metadataTable = db.getStore().getMetadataTable(); metadataTable.delete(OzoneConsts.BLOCK_COUNT); @@ -239,8 +237,7 @@ public void testDelete() throws Exception { final long expectedRegularBlocks = TestDB.KEY_COUNT - numBlocksToDelete; - KeyValueContainerData kvData = newKvData(); - try(ReferenceCountedDB refCountedDB = BlockUtils.getDB(kvData, conf)) { + try(ReferenceCountedDB refCountedDB = BlockUtils.getDB(newKvData(), conf)) { // Test results via block iteration. assertEquals(expectedDeletingBlocks, countDeletingBlocks(refCountedDB)); @@ -272,29 +269,29 @@ public void testDelete() throws Exception { */ @Test public void testReadDeletedBlockChunkInfo() throws Exception { - KeyValueContainerData kvData = newKvData(); - List> deletedBlocks; - Set preUpgradeBlocks = new HashSet<>(); - try(ReferenceCountedDB refCountedDB = BlockUtils.getDB(kvData, conf)) { + try(ReferenceCountedDB refCountedDB = BlockUtils.getDB(newKvData(), conf)) { // Read blocks that were already deleted before the upgrade. - deletedBlocks = refCountedDB.getStore() - .getDeletedBlocksTable().getRangeKVs(null, 100); + List> deletedBlocks = + refCountedDB.getStore() + .getDeletedBlocksTable().getRangeKVs(null, 100); - for (Table.KeyValue chunkListKV : deletedBlocks) { + Set preUpgradeBlocks = new HashSet<>(); + + for(Table.KeyValue chunkListKV: deletedBlocks) { preUpgradeBlocks.add(chunkListKV.getKey()); try { chunkListKV.getValue(); Assert.fail("No exception thrown when trying to retrieve old " + - "deleted blocks values as chunk lists."); - } catch (IOException ex) { + "deleted blocks values as chunk lists."); + } catch(IOException ex) { // Exception thrown as expected. } } - } - runBlockDeletingService(); + Assert.assertEquals(TestDB.NUM_DELETED_BLOCKS, preUpgradeBlocks.size()); + + runBlockDeletingService(); - try(ReferenceCountedDB refCountedDB = BlockUtils.getDB(kvData, conf)) { // After the block deleting service runs, get the updated list of // deleted blocks. deletedBlocks = refCountedDB.getStore() @@ -497,10 +494,6 @@ private OzoneContainer makeMockOzoneContainer() throws Exception { * @throws IOException */ private KeyValueContainerData newKvData() throws IOException { - // The parseKVContainerData() method opens RocksDB without using the - // container cache, so we need to ensure the DB is not already open - // by purging the cache before opening it. - BlockUtils.shutdownCache(ContainerCache.getInstance(conf)); KeyValueContainerData kvData = (KeyValueContainerData) ContainerDataYaml.readContainerFile(containerFile); From c53b02867bfd403b57ce48bcf1278ded526ce298 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Mon, 9 Nov 2020 16:02:32 +0000 Subject: [PATCH 6/6] Fixed typo --- .../apache/hadoop/ozone/container/metadata/DatanodeStore.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStore.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStore.java index 992bce70e3a1..5a0ce7a64623 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStore.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStore.java @@ -47,7 +47,7 @@ public interface DatanodeStore { */ void stop() throws Exception; - /**TestSchemaOneBackwardsCompatibility.java + /** * Get datanode store. * * @return datanode store.