From 95836d2443ea4774becc7f2003e3ea49239780d2 Mon Sep 17 00:00:00 2001 From: Sammi Chen Date: Mon, 29 Jun 2020 16:18:11 +0800 Subject: [PATCH 01/11] HDDS-3892. Datanode initialization is too slow when there are thousands of containers per volume. --- .../common/utils/ContainerCache.java | 49 +++++++++++++------ .../container/ozoneimpl/ContainerReader.java | 2 + 2 files changed, 36 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 d2d29018b32a..e4bb1f3e24a4 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 @@ -117,28 +117,47 @@ 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(); + db = new ReferenceCountedDB(metadataStore, containerDBPath); } catch (Exception e) { LOG.error("Error opening DB. Container:{} ContainerPath:{}", containerID, containerDBPath, e); throw e; + } + + lock.lock(); + try { + ReferenceCountedDB currentDB = + (ReferenceCountedDB) this.get(containerDBPath); + if (currentDB != null) { + // increment the reference before returning the object + currentDB.incrementReference(); + return currentDB; + } else { + this.put(containerDBPath, db); + // increment the reference before returning the object + db.incrementReference(); + return db; + } } finally { lock.unlock(); } diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java index 1b9b3d690724..fa63cf1b862e 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java @@ -120,6 +120,7 @@ public boolean accept(File pathname) { return; } + LOG.info("Start to verify containers on volume {}", hddsVolumeRootDir); for (File scmLoc : scmDir) { File currentDir = new File(scmLoc, Storage.STORAGE_DIR_CURRENT); File[] containerTopDirs = currentDir.listFiles(); @@ -144,6 +145,7 @@ public boolean accept(File pathname) { } } } + LOG.info("Finish verifying containers on volume {}", hddsVolumeRootDir); } private void verifyContainerFile(long containerID, File containerFile) { From 9dda984a9c1a588fc2d8a55a61c7a74b01e62be8 Mon Sep 17 00:00:00 2001 From: Sammi Chen Date: Tue, 30 Jun 2020 11:30:46 +0800 Subject: [PATCH 02/11] release db resource --- .../hadoop/ozone/container/common/utils/ContainerCache.java | 2 ++ 1 file changed, 2 insertions(+) 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 e4bb1f3e24a4..6ca7938cc23b 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 @@ -151,6 +151,8 @@ public ReferenceCountedDB getDB(long containerID, String containerDBType, if (currentDB != null) { // increment the reference before returning the object currentDB.incrementReference(); + // clean the db created in previous step + db.cleanup(); return currentDB; } else { this.put(containerDBPath, db); From 5dd9a0fda5f345f6e71d3fa0cdec07e4e1f197c4 Mon Sep 17 00:00:00 2001 From: Sammi Chen Date: Thu, 9 Jul 2020 17:56:05 +0800 Subject: [PATCH 03/11] address concurrent rocksdb open case --- .../common/utils/ContainerCache.java | 74 +++++++++---------- .../keyvalue/helpers/BlockUtils.java | 22 +++++- .../helpers/KeyValueContainerUtil.java | 18 ++++- .../container/ozoneimpl/ContainerReader.java | 3 +- .../container/common/TestContainerCache.java | 54 +++++++++++++- .../ozoneimpl/TestContainerReader.java | 70 +++++++++++++++++- 6 files changed, 192 insertions(+), 49 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 6ca7938cc23b..1a564e495402 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 @@ -113,55 +113,55 @@ protected boolean removeLRU(LinkEntry entry) { * @return ReferenceCountedDB. */ public ReferenceCountedDB getDB(long containerID, String containerDBType, - String containerDBPath, ConfigurationSource conf) + String containerDBPath, ConfigurationSource conf) + throws IOException { + return getDB(containerID, containerDBType, containerDBPath, conf, true); + } + /** + * Returns a DB handle if available, create the handler otherwise. + * + * @param containerID - ID of the container. + * @param containerDBType - DB type of the container. + * @param containerDBPath - DB path of the container. + * @param acquireLock - false only for one-time ContainerReader execution + * during datanode initialization. Don't set it to false + * in other cases. + * @param conf - Hadoop Configuration. + * @return ReferenceCountedDB. + */ + public ReferenceCountedDB getDB(long containerID, String containerDBType, + String containerDBPath, ConfigurationSource conf, boolean acquireLock) throws IOException { Preconditions.checkState(containerID >= 0, "Container ID cannot be negative."); ReferenceCountedDB db; - lock.lock(); + if (acquireLock) { + lock.lock(); + } try { db = (ReferenceCountedDB) this.get(containerDBPath); - if (db != null) { - db.incrementReference(); - return db; + 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); } - } finally { - lock.unlock(); - } - - try { - MetadataStore metadataStore = - MetadataStoreBuilder.newBuilder() - .setDbFile(new File(containerDBPath)) - .setCreateIfMissing(false) - .setConf(conf) - .setDBType(containerDBType) - .build(); - db = new ReferenceCountedDB(metadataStore, containerDBPath); + // increment the reference before returning the object + db.incrementReference(); + return db; } catch (Exception e) { LOG.error("Error opening DB. Container:{} ContainerPath:{}", containerID, containerDBPath, e); throw e; - } - - lock.lock(); - try { - ReferenceCountedDB currentDB = - (ReferenceCountedDB) this.get(containerDBPath); - if (currentDB != null) { - // increment the reference before returning the object - currentDB.incrementReference(); - // clean the db created in previous step - db.cleanup(); - return currentDB; - } else { - this.put(containerDBPath, db); - // increment the reference before returning the object - db.incrementReference(); - return db; - } } finally { - lock.unlock(); + if (acquireLock) { + lock.unlock(); + } } } 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 99ead01a7f01..a563745b60a3 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 @@ -41,6 +41,23 @@ public final class BlockUtils { private BlockUtils() { } + + /** + * Get a DB handler for a given container. + * If the handler doesn't exist in cache yet, first create one and + * add into cache. This function is called with containerManager + * ReadLock held. + * + * @param containerData containerData. + * @param conf configuration. + * @return MetadataStore handle. + * @throws StorageContainerException + */ + public static ReferenceCountedDB getDB(KeyValueContainerData containerData, + ConfigurationSource conf) throws + StorageContainerException { + return getDB(containerData, conf, true); + } /** * Get a DB handler for a given container. * If the handler doesn't exist in cache yet, first create one and @@ -49,11 +66,12 @@ private BlockUtils() { * * @param containerData containerData. * @param conf configuration. + * @param acquireLock carefully set this to false * @return MetadataStore handle. * @throws StorageContainerException */ public static ReferenceCountedDB getDB(KeyValueContainerData containerData, - ConfigurationSource conf) throws + ConfigurationSource conf, boolean acquireLock) throws StorageContainerException { Preconditions.checkNotNull(containerData); ContainerCache cache = ContainerCache.getInstance(conf); @@ -62,7 +80,7 @@ public static ReferenceCountedDB getDB(KeyValueContainerData containerData, try { return cache.getDB(containerData.getContainerID(), containerData .getContainerDBType(), containerData.getDbFile().getAbsolutePath(), - conf); + conf, acquireLock); } catch (IOException ex) { String message = String.format("Error opening DB. Container:%s " + "ContainerPath:%s", containerData.getContainerID(), containerData 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 2141bed143a1..6a8e445d4eeb 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 @@ -142,7 +142,21 @@ public static void removeContainer(KeyValueContainerData containerData, * @throws IOException */ public static void parseKVContainerData(KeyValueContainerData kvContainerData, - ConfigurationSource config) throws IOException { + ConfigurationSource config) throws IOException { + parseKVContainerData(kvContainerData, config, true); + } + /** + * Parse KeyValueContainerData and verify checksum. Set block related + * metadata like block commit sequence id, block count, bytes used and + * pending delete block count and delete transaction id. + * @param kvContainerData + * @param config + * @param acquireContainerCacheLock carefully set this to false + * @throws IOException + */ + public static void parseKVContainerData(KeyValueContainerData kvContainerData, + ConfigurationSource config, boolean acquireContainerCacheLock) + throws IOException { long containerID = kvContainerData.getContainerID(); File metadataPath = new File(kvContainerData.getMetadataPath()); @@ -164,7 +178,7 @@ public static void parseKVContainerData(KeyValueContainerData kvContainerData, boolean isBlockMetadataSet = false; try(ReferenceCountedDB containerDB = BlockUtils.getDB(kvContainerData, - config)) { + config, acquireContainerCacheLock)) { // Set pending deleted block count. byte[] pendingDeleteBlockCount = diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java index fa63cf1b862e..a28dad2ab201 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java @@ -180,7 +180,8 @@ public void verifyAndFixupContainerData(ContainerData containerData) containerData; containerData.setVolume(hddsVolume); - KeyValueContainerUtil.parseKVContainerData(kvContainerData, config); + KeyValueContainerUtil.parseKVContainerData( + kvContainerData, config, false); KeyValueContainer kvContainer = new KeyValueContainer( kvContainerData, config); diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java index 947a087cb82e..497c81eaed15 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java @@ -31,7 +31,13 @@ import org.junit.rules.ExpectedException; import java.io.File; - +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; /** * Test ContainerCache with evictions. @@ -58,17 +64,18 @@ private void createContainerDB(OzoneConfiguration conf, File dbFile) public void testContainerCacheEviction() throws Exception { File root = new File(testRoot); root.mkdirs(); + root.deleteOnExit(); OzoneConfiguration conf = new OzoneConfiguration(); conf.setInt(OzoneConfigKeys.OZONE_CONTAINER_CACHE_SIZE, 2); ContainerCache cache = ContainerCache.getInstance(conf); + cache.clear(); File containerDir1 = new File(root, "cont1"); File containerDir2 = new File(root, "cont2"); File containerDir3 = new File(root, "cont3"); File containerDir4 = new File(root, "cont4"); - createContainerDB(conf, containerDir1); createContainerDB(conf, containerDir2); createContainerDB(conf, containerDir3); @@ -118,9 +125,50 @@ public void testContainerCacheEviction() throws Exception { db5.close(); db4.close(); - // Decrementing reference count below zero should fail. thrown.expect(IllegalArgumentException.class); db5.close(); } + + @Test + public void testConcurrentDBGet() throws Exception { + File root = new File(testRoot); + root.mkdirs(); + root.deleteOnExit(); + + OzoneConfiguration conf = new OzoneConfiguration(); + conf.setInt(OzoneConfigKeys.OZONE_CONTAINER_CACHE_SIZE, 2); + ContainerCache cache = ContainerCache.getInstance(conf); + cache.clear(); + File containerDir = new File(root, "cont1"); + createContainerDB(conf, containerDir); + ExecutorService executorService = Executors.newFixedThreadPool(2); + Runnable task = () -> { + try { + ReferenceCountedDB db1 = cache.getDB(1, "RocksDB", + containerDir.getPath(), conf); + Assert.assertTrue(db1 != null); + } catch (IOException e) { + Assert.fail("Should get the DB instance"); + } + }; + List futureList = new ArrayList<>(); + futureList.add(executorService.submit(task)); + futureList.add(executorService.submit(task)); + for (Future future: futureList) { + try { + future.get(); + } catch (InterruptedException|ExecutionException e) { + Assert.fail("Should get the DB instance"); + } + } + + ReferenceCountedDB db = cache.getDB(1, "RocksDB", + containerDir.getPath(), conf); + System.out.println(db.getReferenceCount()); + db.close(); + db.close(); + db.close(); + db.cleanup(); + } } 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 e1c5f33ff4af..221287f0b10b 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 @@ -22,15 +22,16 @@ import org.apache.hadoop.conf.StorageUnit; import org.apache.hadoop.hdds.StringUtils; import org.apache.hadoop.hdds.client.BlockID; -import org.apache.hadoop.hdds.conf.ConfigurationSource; import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos; +import org.apache.hadoop.hdds.scm.ScmConfigKeys; import org.apache.hadoop.ozone.OzoneConsts; import org.apache.hadoop.ozone.container.common.helpers.BlockData; import org.apache.hadoop.ozone.container.common.helpers.ChunkInfo; import org.apache.hadoop.ozone.container.common.impl.ChunkLayOutVersion; import org.apache.hadoop.ozone.container.common.impl.ContainerSet; import org.apache.hadoop.ozone.container.common.interfaces.Container; +import org.apache.hadoop.ozone.container.common.utils.ContainerCache; import org.apache.hadoop.ozone.container.common.utils.ReferenceCountedDB; import org.apache.hadoop.ozone.container.common.volume.HddsVolume; import org.apache.hadoop.ozone.container.common.volume.MutableVolumeSet; @@ -68,7 +69,7 @@ public class TestContainerReader { private MutableVolumeSet volumeSet; private HddsVolume hddsVolume; private ContainerSet containerSet; - private ConfigurationSource conf; + private OzoneConfiguration conf; private RoundRobinVolumeChoosingPolicy volumeChoosingPolicy; @@ -123,7 +124,7 @@ public void setup() throws Exception { private void markBlocksForDelete(KeyValueContainer keyValueContainer, boolean setMetaData, List blockNames, int count) throws Exception { try(ReferenceCountedDB metadataStore = BlockUtils.getDB(keyValueContainer - .getContainerData(), conf)) { + .getContainerData(), conf, false)) { for (int i = 0; i < count; i++) { byte[] blkBytes = Longs.toByteArray(blockNames.get(i)); @@ -160,7 +161,7 @@ private List addBlocks(KeyValueContainer keyValueContainer, List blkNames = new ArrayList<>(); try(ReferenceCountedDB metadataStore = BlockUtils.getDB(keyValueContainer - .getContainerData(), conf)) { + .getContainerData(), conf, false)) { for (int i = 0; i < blockCount; i++) { // Creating BlockData @@ -219,4 +220,65 @@ public void testContainerReader() throws Exception { keyValueContainerData.getNumPendingDeletionBlocks()); } } + + @Test + public void testMultipleContainerReader() throws Exception { + final int volumeNum = 10; + StringBuffer datanodeDirs = new StringBuffer(); + File[] volumeDirs = new File[volumeNum]; + for (int i = 0; i < volumeNum; i++) { + volumeDirs[i] = tempDir.newFolder(); + datanodeDirs = datanodeDirs.append(volumeDirs[i]).append(","); + } + conf.set(ScmConfigKeys.HDDS_DATANODE_DIR_KEY, + datanodeDirs.toString()); + MutableVolumeSet volumeSets = + new MutableVolumeSet(datanodeId.toString(), conf); + ContainerCache cache = ContainerCache.getInstance(conf); + cache.clear(); + + RoundRobinVolumeChoosingPolicy policy = + new RoundRobinVolumeChoosingPolicy(); + + blockCount *= volumeNum; + final int containerCount = 100; + for (int i = 0; i < containerCount; i++) { + KeyValueContainerData keyValueContainerData = + new KeyValueContainerData(i, ChunkLayOutVersion.FILE_PER_BLOCK, + (long) StorageUnit.GB.toBytes(5), UUID.randomUUID().toString(), + datanodeId.toString()); + + KeyValueContainer keyValueContainer = + new KeyValueContainer(keyValueContainerData, + conf); + keyValueContainer.create(volumeSets, policy, scmId); + + List blkNames; + if (i % 2 == 0) { + blkNames = addBlocks(keyValueContainer, true); + markBlocksForDelete(keyValueContainer, true, blkNames, i); + } else { + blkNames = addBlocks(keyValueContainer, false); + markBlocksForDelete(keyValueContainer, false, blkNames, i); + } + } + + List hddsVolumes = volumeSets.getVolumesList(); + ContainerReader[] containerReaders = new ContainerReader[volumeNum]; + Thread[] threads = new Thread[volumeNum]; + for (int i = 0; i < volumeNum; i++) { + containerReaders[i] = new ContainerReader(volumeSets, + hddsVolumes.get(i), containerSet, conf); + threads[i] = new Thread(containerReaders[i]); + } + for (int i = 0; i < volumeNum; i++) { + threads[i].start(); + } + for (int i = 0; i < volumeNum; i++) { + threads[i].join(); + } + Assert.assertTrue( + containerSet.getContainerMap().entrySet().size() == containerCount); + Assert.assertTrue(cache.size() == containerCount); + } } From 15ec4d3c266dc13f5ce9d1b847a93119c73d389f Mon Sep 17 00:00:00 2001 From: Sammi Chen Date: Thu, 9 Jul 2020 19:28:56 +0800 Subject: [PATCH 04/11] fix checkstyle --- .../ozone/container/keyvalue/helpers/KeyValueContainerUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 6a8e445d4eeb..4ca03b60d3ed 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 @@ -142,7 +142,7 @@ public static void removeContainer(KeyValueContainerData containerData, * @throws IOException */ public static void parseKVContainerData(KeyValueContainerData kvContainerData, - ConfigurationSource config) throws IOException { + ConfigurationSource config) throws IOException { parseKVContainerData(kvContainerData, config, true); } /** From 20b694ad0d6155df0eb9b5091ed3d6caef85ab37 Mon Sep 17 00:00:00 2001 From: Sammi Chen Date: Thu, 16 Jul 2020 20:22:31 +0800 Subject: [PATCH 05/11] Revert "fix checkstyle" This reverts commit 15ec4d3c266dc13f5ce9d1b847a93119c73d389f. --- .../ozone/container/keyvalue/helpers/KeyValueContainerUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 4ca03b60d3ed..6a8e445d4eeb 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 @@ -142,7 +142,7 @@ public static void removeContainer(KeyValueContainerData containerData, * @throws IOException */ public static void parseKVContainerData(KeyValueContainerData kvContainerData, - ConfigurationSource config) throws IOException { + ConfigurationSource config) throws IOException { parseKVContainerData(kvContainerData, config, true); } /** From fa37b9f398aa089cb023d813c5a6aebd4ac2d6ad Mon Sep 17 00:00:00 2001 From: Sammi Chen Date: Thu, 16 Jul 2020 20:22:42 +0800 Subject: [PATCH 06/11] Revert "address concurrent rocksdb open case" This reverts commit 5dd9a0fda5f345f6e71d3fa0cdec07e4e1f197c4. --- .../common/utils/ContainerCache.java | 74 +++++++++---------- .../keyvalue/helpers/BlockUtils.java | 22 +----- .../helpers/KeyValueContainerUtil.java | 18 +---- .../container/ozoneimpl/ContainerReader.java | 3 +- .../container/common/TestContainerCache.java | 54 +------------- .../ozoneimpl/TestContainerReader.java | 70 +----------------- 6 files changed, 49 insertions(+), 192 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 1a564e495402..6ca7938cc23b 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 @@ -113,55 +113,55 @@ protected boolean removeLRU(LinkEntry entry) { * @return ReferenceCountedDB. */ public ReferenceCountedDB getDB(long containerID, String containerDBType, - String containerDBPath, ConfigurationSource conf) - throws IOException { - return getDB(containerID, containerDBType, containerDBPath, conf, true); - } - /** - * Returns a DB handle if available, create the handler otherwise. - * - * @param containerID - ID of the container. - * @param containerDBType - DB type of the container. - * @param containerDBPath - DB path of the container. - * @param acquireLock - false only for one-time ContainerReader execution - * during datanode initialization. Don't set it to false - * in other cases. - * @param conf - Hadoop Configuration. - * @return ReferenceCountedDB. - */ - public ReferenceCountedDB getDB(long containerID, String containerDBType, - String containerDBPath, ConfigurationSource conf, boolean acquireLock) + String containerDBPath, ConfigurationSource conf) throws IOException { Preconditions.checkState(containerID >= 0, "Container ID cannot be negative."); ReferenceCountedDB db; - if (acquireLock) { - lock.lock(); - } + lock.lock(); try { 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); + 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(); + db = new ReferenceCountedDB(metadataStore, containerDBPath); } catch (Exception e) { LOG.error("Error opening DB. Container:{} ContainerPath:{}", containerID, containerDBPath, e); throw e; - } finally { - if (acquireLock) { - lock.unlock(); + } + + lock.lock(); + try { + ReferenceCountedDB currentDB = + (ReferenceCountedDB) this.get(containerDBPath); + if (currentDB != null) { + // increment the reference before returning the object + currentDB.incrementReference(); + // clean the db created in previous step + db.cleanup(); + return currentDB; + } else { + this.put(containerDBPath, db); + // increment the reference before returning the object + db.incrementReference(); + return db; } + } finally { + lock.unlock(); } } 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 a563745b60a3..99ead01a7f01 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 @@ -41,23 +41,6 @@ public final class BlockUtils { private BlockUtils() { } - - /** - * Get a DB handler for a given container. - * If the handler doesn't exist in cache yet, first create one and - * add into cache. This function is called with containerManager - * ReadLock held. - * - * @param containerData containerData. - * @param conf configuration. - * @return MetadataStore handle. - * @throws StorageContainerException - */ - public static ReferenceCountedDB getDB(KeyValueContainerData containerData, - ConfigurationSource conf) throws - StorageContainerException { - return getDB(containerData, conf, true); - } /** * Get a DB handler for a given container. * If the handler doesn't exist in cache yet, first create one and @@ -66,12 +49,11 @@ public static ReferenceCountedDB getDB(KeyValueContainerData containerData, * * @param containerData containerData. * @param conf configuration. - * @param acquireLock carefully set this to false * @return MetadataStore handle. * @throws StorageContainerException */ public static ReferenceCountedDB getDB(KeyValueContainerData containerData, - ConfigurationSource conf, boolean acquireLock) throws + ConfigurationSource conf) throws StorageContainerException { Preconditions.checkNotNull(containerData); ContainerCache cache = ContainerCache.getInstance(conf); @@ -80,7 +62,7 @@ public static ReferenceCountedDB getDB(KeyValueContainerData containerData, try { return cache.getDB(containerData.getContainerID(), containerData .getContainerDBType(), containerData.getDbFile().getAbsolutePath(), - conf, acquireLock); + conf); } catch (IOException ex) { String message = String.format("Error opening DB. Container:%s " + "ContainerPath:%s", containerData.getContainerID(), containerData 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 6a8e445d4eeb..2141bed143a1 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 @@ -142,21 +142,7 @@ public static void removeContainer(KeyValueContainerData containerData, * @throws IOException */ public static void parseKVContainerData(KeyValueContainerData kvContainerData, - ConfigurationSource config) throws IOException { - parseKVContainerData(kvContainerData, config, true); - } - /** - * Parse KeyValueContainerData and verify checksum. Set block related - * metadata like block commit sequence id, block count, bytes used and - * pending delete block count and delete transaction id. - * @param kvContainerData - * @param config - * @param acquireContainerCacheLock carefully set this to false - * @throws IOException - */ - public static void parseKVContainerData(KeyValueContainerData kvContainerData, - ConfigurationSource config, boolean acquireContainerCacheLock) - throws IOException { + ConfigurationSource config) throws IOException { long containerID = kvContainerData.getContainerID(); File metadataPath = new File(kvContainerData.getMetadataPath()); @@ -178,7 +164,7 @@ public static void parseKVContainerData(KeyValueContainerData kvContainerData, boolean isBlockMetadataSet = false; try(ReferenceCountedDB containerDB = BlockUtils.getDB(kvContainerData, - config, acquireContainerCacheLock)) { + config)) { // Set pending deleted block count. byte[] pendingDeleteBlockCount = diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java index a28dad2ab201..fa63cf1b862e 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerReader.java @@ -180,8 +180,7 @@ public void verifyAndFixupContainerData(ContainerData containerData) containerData; containerData.setVolume(hddsVolume); - KeyValueContainerUtil.parseKVContainerData( - kvContainerData, config, false); + KeyValueContainerUtil.parseKVContainerData(kvContainerData, config); KeyValueContainer kvContainer = new KeyValueContainer( kvContainerData, config); diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java index 497c81eaed15..947a087cb82e 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java @@ -31,13 +31,7 @@ import org.junit.rules.ExpectedException; import java.io.File; -import java.io.IOException; -import java.util.ArrayList; -import java.util.List; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.Future; + /** * Test ContainerCache with evictions. @@ -64,18 +58,17 @@ private void createContainerDB(OzoneConfiguration conf, File dbFile) public void testContainerCacheEviction() throws Exception { File root = new File(testRoot); root.mkdirs(); - root.deleteOnExit(); OzoneConfiguration conf = new OzoneConfiguration(); conf.setInt(OzoneConfigKeys.OZONE_CONTAINER_CACHE_SIZE, 2); ContainerCache cache = ContainerCache.getInstance(conf); - cache.clear(); File containerDir1 = new File(root, "cont1"); File containerDir2 = new File(root, "cont2"); File containerDir3 = new File(root, "cont3"); File containerDir4 = new File(root, "cont4"); + createContainerDB(conf, containerDir1); createContainerDB(conf, containerDir2); createContainerDB(conf, containerDir3); @@ -125,50 +118,9 @@ public void testContainerCacheEviction() throws Exception { db5.close(); db4.close(); + // Decrementing reference count below zero should fail. thrown.expect(IllegalArgumentException.class); db5.close(); } - - @Test - public void testConcurrentDBGet() throws Exception { - File root = new File(testRoot); - root.mkdirs(); - root.deleteOnExit(); - - OzoneConfiguration conf = new OzoneConfiguration(); - conf.setInt(OzoneConfigKeys.OZONE_CONTAINER_CACHE_SIZE, 2); - ContainerCache cache = ContainerCache.getInstance(conf); - cache.clear(); - File containerDir = new File(root, "cont1"); - createContainerDB(conf, containerDir); - ExecutorService executorService = Executors.newFixedThreadPool(2); - Runnable task = () -> { - try { - ReferenceCountedDB db1 = cache.getDB(1, "RocksDB", - containerDir.getPath(), conf); - Assert.assertTrue(db1 != null); - } catch (IOException e) { - Assert.fail("Should get the DB instance"); - } - }; - List futureList = new ArrayList<>(); - futureList.add(executorService.submit(task)); - futureList.add(executorService.submit(task)); - for (Future future: futureList) { - try { - future.get(); - } catch (InterruptedException|ExecutionException e) { - Assert.fail("Should get the DB instance"); - } - } - - ReferenceCountedDB db = cache.getDB(1, "RocksDB", - containerDir.getPath(), conf); - System.out.println(db.getReferenceCount()); - db.close(); - db.close(); - db.close(); - db.cleanup(); - } } 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 221287f0b10b..e1c5f33ff4af 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 @@ -22,16 +22,15 @@ import org.apache.hadoop.conf.StorageUnit; import org.apache.hadoop.hdds.StringUtils; import org.apache.hadoop.hdds.client.BlockID; +import org.apache.hadoop.hdds.conf.ConfigurationSource; import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos; -import org.apache.hadoop.hdds.scm.ScmConfigKeys; import org.apache.hadoop.ozone.OzoneConsts; import org.apache.hadoop.ozone.container.common.helpers.BlockData; import org.apache.hadoop.ozone.container.common.helpers.ChunkInfo; import org.apache.hadoop.ozone.container.common.impl.ChunkLayOutVersion; import org.apache.hadoop.ozone.container.common.impl.ContainerSet; import org.apache.hadoop.ozone.container.common.interfaces.Container; -import org.apache.hadoop.ozone.container.common.utils.ContainerCache; import org.apache.hadoop.ozone.container.common.utils.ReferenceCountedDB; import org.apache.hadoop.ozone.container.common.volume.HddsVolume; import org.apache.hadoop.ozone.container.common.volume.MutableVolumeSet; @@ -69,7 +68,7 @@ public class TestContainerReader { private MutableVolumeSet volumeSet; private HddsVolume hddsVolume; private ContainerSet containerSet; - private OzoneConfiguration conf; + private ConfigurationSource conf; private RoundRobinVolumeChoosingPolicy volumeChoosingPolicy; @@ -124,7 +123,7 @@ public void setup() throws Exception { private void markBlocksForDelete(KeyValueContainer keyValueContainer, boolean setMetaData, List blockNames, int count) throws Exception { try(ReferenceCountedDB metadataStore = BlockUtils.getDB(keyValueContainer - .getContainerData(), conf, false)) { + .getContainerData(), conf)) { for (int i = 0; i < count; i++) { byte[] blkBytes = Longs.toByteArray(blockNames.get(i)); @@ -161,7 +160,7 @@ private List addBlocks(KeyValueContainer keyValueContainer, List blkNames = new ArrayList<>(); try(ReferenceCountedDB metadataStore = BlockUtils.getDB(keyValueContainer - .getContainerData(), conf, false)) { + .getContainerData(), conf)) { for (int i = 0; i < blockCount; i++) { // Creating BlockData @@ -220,65 +219,4 @@ public void testContainerReader() throws Exception { keyValueContainerData.getNumPendingDeletionBlocks()); } } - - @Test - public void testMultipleContainerReader() throws Exception { - final int volumeNum = 10; - StringBuffer datanodeDirs = new StringBuffer(); - File[] volumeDirs = new File[volumeNum]; - for (int i = 0; i < volumeNum; i++) { - volumeDirs[i] = tempDir.newFolder(); - datanodeDirs = datanodeDirs.append(volumeDirs[i]).append(","); - } - conf.set(ScmConfigKeys.HDDS_DATANODE_DIR_KEY, - datanodeDirs.toString()); - MutableVolumeSet volumeSets = - new MutableVolumeSet(datanodeId.toString(), conf); - ContainerCache cache = ContainerCache.getInstance(conf); - cache.clear(); - - RoundRobinVolumeChoosingPolicy policy = - new RoundRobinVolumeChoosingPolicy(); - - blockCount *= volumeNum; - final int containerCount = 100; - for (int i = 0; i < containerCount; i++) { - KeyValueContainerData keyValueContainerData = - new KeyValueContainerData(i, ChunkLayOutVersion.FILE_PER_BLOCK, - (long) StorageUnit.GB.toBytes(5), UUID.randomUUID().toString(), - datanodeId.toString()); - - KeyValueContainer keyValueContainer = - new KeyValueContainer(keyValueContainerData, - conf); - keyValueContainer.create(volumeSets, policy, scmId); - - List blkNames; - if (i % 2 == 0) { - blkNames = addBlocks(keyValueContainer, true); - markBlocksForDelete(keyValueContainer, true, blkNames, i); - } else { - blkNames = addBlocks(keyValueContainer, false); - markBlocksForDelete(keyValueContainer, false, blkNames, i); - } - } - - List hddsVolumes = volumeSets.getVolumesList(); - ContainerReader[] containerReaders = new ContainerReader[volumeNum]; - Thread[] threads = new Thread[volumeNum]; - for (int i = 0; i < volumeNum; i++) { - containerReaders[i] = new ContainerReader(volumeSets, - hddsVolumes.get(i), containerSet, conf); - threads[i] = new Thread(containerReaders[i]); - } - for (int i = 0; i < volumeNum; i++) { - threads[i].start(); - } - for (int i = 0; i < volumeNum; i++) { - threads[i].join(); - } - Assert.assertTrue( - containerSet.getContainerMap().entrySet().size() == containerCount); - Assert.assertTrue(cache.size() == containerCount); - } } From 71b4571af9985d47c0d2ec5859ed5c7f6f7a4892 Mon Sep 17 00:00:00 2001 From: Sammi Chen Date: Fri, 17 Jul 2020 10:52:23 +0800 Subject: [PATCH 07/11] Use striped lock --- .../common/utils/ContainerCache.java | 84 ++++++++++--------- .../container/ozoneimpl/OzoneContainer.java | 3 + .../container/common/TestContainerCache.java | 52 ++++++++++++ .../ozoneimpl/TestContainerReader.java | 69 ++++++++++++++- 4 files changed, 168 insertions(+), 40 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 6ca7938cc23b..1a913e9662aa 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 @@ -23,6 +23,7 @@ import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; +import com.google.common.util.concurrent.Striped; import org.apache.hadoop.hdds.conf.ConfigurationSource; import org.apache.hadoop.hdds.utils.MetadataStore; import org.apache.hadoop.hdds.utils.MetadataStoreBuilder; @@ -43,6 +44,7 @@ public final class ContainerCache extends LRUMap { private final Lock lock = new ReentrantLock(); private static ContainerCache cache; private static final float LOAD_FACTOR = 0.75f; + private final static Striped rocksDBLock = Striped.lazyWeakLock(1024); /** * Constructs a cache that holds DBHandle references. */ @@ -118,50 +120,56 @@ public ReferenceCountedDB getDB(long containerID, String containerDBType, Preconditions.checkState(containerID >= 0, "Container ID cannot be negative."); ReferenceCountedDB db; - lock.lock(); + Lock containerLock = rocksDBLock.get(containerDBPath); + containerLock.lock(); try { - db = (ReferenceCountedDB) this.get(containerDBPath); - if (db != null) { - db.incrementReference(); - return db; + lock.lock(); + try { + db = (ReferenceCountedDB) this.get(containerDBPath); + if (db != null) { + db.incrementReference(); + return db; + } + } finally { + lock.unlock(); } - } finally { - lock.unlock(); - } - try { - MetadataStore metadataStore = - MetadataStoreBuilder.newBuilder() - .setDbFile(new File(containerDBPath)) - .setCreateIfMissing(false) - .setConf(conf) - .setDBType(containerDBType) - .build(); - db = new ReferenceCountedDB(metadataStore, containerDBPath); - } catch (Exception e) { - LOG.error("Error opening DB. Container:{} ContainerPath:{}", - containerID, containerDBPath, e); - throw e; - } + try { + MetadataStore metadataStore = + MetadataStoreBuilder.newBuilder() + .setDbFile(new File(containerDBPath)) + .setCreateIfMissing(false) + .setConf(conf) + .setDBType(containerDBType) + .build(); + db = new ReferenceCountedDB(metadataStore, containerDBPath); + } catch (Exception e) { + LOG.error("Error opening DB. Container:{} ContainerPath:{}", + containerID, containerDBPath, e); + throw e; + } - lock.lock(); - try { - ReferenceCountedDB currentDB = - (ReferenceCountedDB) this.get(containerDBPath); - if (currentDB != null) { - // increment the reference before returning the object - currentDB.incrementReference(); - // clean the db created in previous step - db.cleanup(); - return currentDB; - } else { - this.put(containerDBPath, db); - // increment the reference before returning the object - db.incrementReference(); - return db; + lock.lock(); + try { + ReferenceCountedDB currentDB = + (ReferenceCountedDB) this.get(containerDBPath); + if (currentDB != null) { + // increment the reference before returning the object + currentDB.incrementReference(); + // clean the db created in previous step + db.cleanup(); + return currentDB; + } else { + this.put(containerDBPath, db); + // increment the reference before returning the object + db.incrementReference(); + return db; + } + } finally { + lock.unlock(); } } finally { - lock.unlock(); + containerLock.unlock(); } } diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java index bbbec25af783..842670f882ca 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java @@ -162,6 +162,7 @@ private void buildContainerSet() { Iterator volumeSetIterator = volumeSet.getVolumesList() .iterator(); ArrayList volumeThreads = new ArrayList(); + long startTime = System.currentTimeMillis(); //TODO: diskchecker should be run before this, to see how disks are. // And also handle disk failure tolerance need to be added @@ -182,6 +183,8 @@ private void buildContainerSet() { Thread.currentThread().interrupt(); } + LOG.info("Build ContainerSet costs {}s", + (System.currentTimeMillis() - startTime) / 1000); } /** diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java index 947a087cb82e..3e61e5b273ed 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java @@ -31,6 +31,13 @@ import org.junit.rules.ExpectedException; import java.io.File; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; /** @@ -63,6 +70,8 @@ public void testContainerCacheEviction() throws Exception { conf.setInt(OzoneConfigKeys.OZONE_CONTAINER_CACHE_SIZE, 2); ContainerCache cache = ContainerCache.getInstance(conf); + cache.clear(); + Assert.assertTrue(cache.size() == 0); File containerDir1 = new File(root, "cont1"); File containerDir2 = new File(root, "cont2"); File containerDir3 = new File(root, "cont3"); @@ -123,4 +132,47 @@ public void testContainerCacheEviction() throws Exception { thrown.expect(IllegalArgumentException.class); db5.close(); } + + @Test + public void testConcurrentDBGet() throws Exception { + File root = new File(testRoot); + root.mkdirs(); + root.deleteOnExit(); + + OzoneConfiguration conf = new OzoneConfiguration(); + conf.setInt(OzoneConfigKeys.OZONE_CONTAINER_CACHE_SIZE, 2); + ContainerCache cache = ContainerCache.getInstance(conf); + cache.clear(); + Assert.assertTrue(cache.size() == 0); + File containerDir = new File(root, "cont1"); + createContainerDB(conf, containerDir); + ExecutorService executorService = Executors.newFixedThreadPool(2); + Runnable task = () -> { + try { + ReferenceCountedDB db1 = cache.getDB(1, "RocksDB", + containerDir.getPath(), conf); + Assert.assertTrue(db1 != null); + } catch (IOException e) { + Assert.fail("Should get the DB instance"); + } + }; + List futureList = new ArrayList<>(); + futureList.add(executorService.submit(task)); + futureList.add(executorService.submit(task)); + for (Future future: futureList) { + try { + future.get(); + } catch (InterruptedException| ExecutionException e) { + Assert.fail("Should get the DB instance"); + } + } + + ReferenceCountedDB db = cache.getDB(1, "RocksDB", + containerDir.getPath(), conf); + db.close(); + db.close(); + db.close(); + Assert.assertTrue(cache.size() == 1); + db.cleanup(); + } } 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 e1c5f33ff4af..adce24767c0c 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 @@ -22,15 +22,16 @@ import org.apache.hadoop.conf.StorageUnit; import org.apache.hadoop.hdds.StringUtils; import org.apache.hadoop.hdds.client.BlockID; -import org.apache.hadoop.hdds.conf.ConfigurationSource; import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos; +import org.apache.hadoop.hdds.scm.ScmConfigKeys; import org.apache.hadoop.ozone.OzoneConsts; import org.apache.hadoop.ozone.container.common.helpers.BlockData; import org.apache.hadoop.ozone.container.common.helpers.ChunkInfo; import org.apache.hadoop.ozone.container.common.impl.ChunkLayOutVersion; import org.apache.hadoop.ozone.container.common.impl.ContainerSet; import org.apache.hadoop.ozone.container.common.interfaces.Container; +import org.apache.hadoop.ozone.container.common.utils.ContainerCache; import org.apache.hadoop.ozone.container.common.utils.ReferenceCountedDB; import org.apache.hadoop.ozone.container.common.volume.HddsVolume; import org.apache.hadoop.ozone.container.common.volume.MutableVolumeSet; @@ -68,7 +69,7 @@ public class TestContainerReader { private MutableVolumeSet volumeSet; private HddsVolume hddsVolume; private ContainerSet containerSet; - private ConfigurationSource conf; + private OzoneConfiguration conf; private RoundRobinVolumeChoosingPolicy volumeChoosingPolicy; @@ -219,4 +220,68 @@ public void testContainerReader() throws Exception { keyValueContainerData.getNumPendingDeletionBlocks()); } } + + @Test + public void testMultipleContainerReader() throws Exception { + final int volumeNum = 10; + StringBuffer datanodeDirs = new StringBuffer(); + File[] volumeDirs = new File[volumeNum]; + for (int i = 0; i < volumeNum; i++) { + volumeDirs[i] = tempDir.newFolder(); + datanodeDirs = datanodeDirs.append(volumeDirs[i]).append(","); + } + conf.set(ScmConfigKeys.HDDS_DATANODE_DIR_KEY, + datanodeDirs.toString()); + MutableVolumeSet volumeSets = + new MutableVolumeSet(datanodeId.toString(), conf); + ContainerCache cache = ContainerCache.getInstance(conf); + cache.clear(); + + RoundRobinVolumeChoosingPolicy policy = + new RoundRobinVolumeChoosingPolicy(); + + final int containerCount = 100; + blockCount = containerCount; + for (int i = 0; i < containerCount; i++) { + KeyValueContainerData keyValueContainerData = + new KeyValueContainerData(i, ChunkLayOutVersion.FILE_PER_BLOCK, + (long) StorageUnit.GB.toBytes(5), UUID.randomUUID().toString(), + datanodeId.toString()); + + KeyValueContainer keyValueContainer = + new KeyValueContainer(keyValueContainerData, + conf); + keyValueContainer.create(volumeSets, policy, scmId); + + List blkNames; + if (i % 2 == 0) { + blkNames = addBlocks(keyValueContainer, true); + markBlocksForDelete(keyValueContainer, true, blkNames, i); + } else { + blkNames = addBlocks(keyValueContainer, false); + markBlocksForDelete(keyValueContainer, false, blkNames, i); + } + } + + List hddsVolumes = volumeSets.getVolumesList(); + ContainerReader[] containerReaders = new ContainerReader[volumeNum]; + Thread[] threads = new Thread[volumeNum]; + for (int i = 0; i < volumeNum; i++) { + containerReaders[i] = new ContainerReader(volumeSets, + hddsVolumes.get(i), containerSet, conf); + threads[i] = new Thread(containerReaders[i]); + } + long startTime = System.currentTimeMillis(); + for (int i = 0; i < volumeNum; i++) { + threads[i].start(); + } + for (int i = 0; i < volumeNum; i++) { + threads[i].join(); + } + System.out.println("Open " + volumeNum + " Volume with " + containerCount + + " costs " + (System.currentTimeMillis() - startTime) / 1000 + "s"); + Assert.assertTrue( + containerSet.getContainerMap().entrySet().size() == containerCount); + Assert.assertTrue(cache.size() == containerCount); + } } From b4060aa590e4e5f7c8a40070e64177abab5ad274 Mon Sep 17 00:00:00 2001 From: Sammi Chen Date: Fri, 17 Jul 2020 11:48:01 +0800 Subject: [PATCH 08/11] checkstyle --- .../hadoop/ozone/container/common/utils/ContainerCache.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 1a913e9662aa..307c90b4368c 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 @@ -44,7 +44,7 @@ public final class ContainerCache extends LRUMap { private final Lock lock = new ReentrantLock(); private static ContainerCache cache; private static final float LOAD_FACTOR = 0.75f; - private final static Striped rocksDBLock = Striped.lazyWeakLock(1024); + private final Striped rocksDBLock = Striped.lazyWeakLock(1024); /** * Constructs a cache that holds DBHandle references. */ From f86391b2c2d2b0b4de18162b88ceb14f97b3b6de Mon Sep 17 00:00:00 2001 From: Sammi Chen Date: Mon, 20 Jul 2020 14:54:56 +0800 Subject: [PATCH 09/11] address comments --- .../java/org/apache/hadoop/ozone/OzoneConfigKeys.java | 3 +++ .../ozone/container/common/utils/ContainerCache.java | 10 +++++++--- .../ozone/container/common/TestContainerCache.java | 8 ++++---- .../ozone/container/ozoneimpl/TestContainerReader.java | 6 +++--- 4 files changed, 17 insertions(+), 10 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java index 7d46b01a6dbf..1567d41ef438 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java @@ -104,6 +104,9 @@ public final class OzoneConfigKeys { public static final String OZONE_CONTAINER_CACHE_SIZE = "ozone.container.cache.size"; public static final int OZONE_CONTAINER_CACHE_DEFAULT = 1024; + public static final String OZONE_CONTAINER_CACHE_LOCK_STRIPS = + "ozone.container.cache.lock.strips"; + public static final int OZONE_CONTAINER_CACHE_LOCK_STRIPS_DEFAULT = 1024; public static final String OZONE_SCM_BLOCK_SIZE = "ozone.scm.block.size"; 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 307c90b4368c..5e8a89dc855a 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 @@ -44,13 +44,14 @@ public final class ContainerCache extends LRUMap { private final Lock lock = new ReentrantLock(); private static ContainerCache cache; private static final float LOAD_FACTOR = 0.75f; - private final Striped rocksDBLock = Striped.lazyWeakLock(1024); + private final Striped rocksDBLock; /** * Constructs a cache that holds DBHandle references. */ - private ContainerCache(int maxSize, float loadFactor, boolean + private ContainerCache(int maxSize, int strips, float loadFactor, boolean scanUntilRemovable) { super(maxSize, loadFactor, scanUntilRemovable); + rocksDBLock = Striped.lazyWeakLock(strips); } /** @@ -65,7 +66,10 @@ public synchronized static ContainerCache getInstance( if (cache == null) { int cacheSize = conf.getInt(OzoneConfigKeys.OZONE_CONTAINER_CACHE_SIZE, OzoneConfigKeys.OZONE_CONTAINER_CACHE_DEFAULT); - cache = new ContainerCache(cacheSize, LOAD_FACTOR, true); + int strips = conf.getInt( + OzoneConfigKeys.OZONE_CONTAINER_CACHE_LOCK_STRIPS, + OzoneConfigKeys.OZONE_CONTAINER_CACHE_LOCK_STRIPS_DEFAULT); + cache = new ContainerCache(cacheSize, strips, LOAD_FACTOR, true); } return cache; } diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java index 3e61e5b273ed..2e389903769f 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestContainerCache.java @@ -71,7 +71,7 @@ public void testContainerCacheEviction() throws Exception { ContainerCache cache = ContainerCache.getInstance(conf); cache.clear(); - Assert.assertTrue(cache.size() == 0); + Assert.assertEquals(0, cache.size()); File containerDir1 = new File(root, "cont1"); File containerDir2 = new File(root, "cont2"); File containerDir3 = new File(root, "cont3"); @@ -143,7 +143,7 @@ public void testConcurrentDBGet() throws Exception { conf.setInt(OzoneConfigKeys.OZONE_CONTAINER_CACHE_SIZE, 2); ContainerCache cache = ContainerCache.getInstance(conf); cache.clear(); - Assert.assertTrue(cache.size() == 0); + Assert.assertEquals(0, cache.size()); File containerDir = new File(root, "cont1"); createContainerDB(conf, containerDir); ExecutorService executorService = Executors.newFixedThreadPool(2); @@ -151,7 +151,7 @@ public void testConcurrentDBGet() throws Exception { try { ReferenceCountedDB db1 = cache.getDB(1, "RocksDB", containerDir.getPath(), conf); - Assert.assertTrue(db1 != null); + Assert.assertNotNull(db1); } catch (IOException e) { Assert.fail("Should get the DB instance"); } @@ -172,7 +172,7 @@ public void testConcurrentDBGet() throws Exception { db.close(); db.close(); db.close(); - Assert.assertTrue(cache.size() == 1); + Assert.assertEquals(1, cache.size()); db.cleanup(); } } 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 adce24767c0c..15c023641321 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 @@ -280,8 +280,8 @@ public void testMultipleContainerReader() throws Exception { } System.out.println("Open " + volumeNum + " Volume with " + containerCount + " costs " + (System.currentTimeMillis() - startTime) / 1000 + "s"); - Assert.assertTrue( - containerSet.getContainerMap().entrySet().size() == containerCount); - Assert.assertTrue(cache.size() == containerCount); + Assert.assertEquals(containerCount, + containerSet.getContainerMap().entrySet().size()); + Assert.assertEquals(containerCount, cache.size()); } } From b53da4da747f419ededba258bdd39bc09fb16c51 Mon Sep 17 00:00:00 2001 From: Sammi Chen Date: Tue, 21 Jul 2020 16:38:03 +0800 Subject: [PATCH 10/11] change strip to stripe --- .../org/apache/hadoop/ozone/OzoneConfigKeys.java | 6 +++--- .../ozone/container/common/utils/ContainerCache.java | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java index 1567d41ef438..0703ff17649e 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java @@ -104,9 +104,9 @@ public final class OzoneConfigKeys { public static final String OZONE_CONTAINER_CACHE_SIZE = "ozone.container.cache.size"; public static final int OZONE_CONTAINER_CACHE_DEFAULT = 1024; - public static final String OZONE_CONTAINER_CACHE_LOCK_STRIPS = - "ozone.container.cache.lock.strips"; - public static final int OZONE_CONTAINER_CACHE_LOCK_STRIPS_DEFAULT = 1024; + public static final String OZONE_CONTAINER_CACHE_LOCK_STRIPES = + "ozone.container.cache.lock.stripes"; + public static final int OZONE_CONTAINER_CACHE_LOCK_STRIPES_DEFAULT = 1024; public static final String OZONE_SCM_BLOCK_SIZE = "ozone.scm.block.size"; 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 5e8a89dc855a..f4d8f43f7065 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 @@ -48,10 +48,10 @@ public final class ContainerCache extends LRUMap { /** * Constructs a cache that holds DBHandle references. */ - private ContainerCache(int maxSize, int strips, float loadFactor, boolean + private ContainerCache(int maxSize, int stripes, float loadFactor, boolean scanUntilRemovable) { super(maxSize, loadFactor, scanUntilRemovable); - rocksDBLock = Striped.lazyWeakLock(strips); + rocksDBLock = Striped.lazyWeakLock(stripes); } /** @@ -66,10 +66,10 @@ public synchronized static ContainerCache getInstance( if (cache == null) { int cacheSize = conf.getInt(OzoneConfigKeys.OZONE_CONTAINER_CACHE_SIZE, OzoneConfigKeys.OZONE_CONTAINER_CACHE_DEFAULT); - int strips = conf.getInt( - OzoneConfigKeys.OZONE_CONTAINER_CACHE_LOCK_STRIPS, - OzoneConfigKeys.OZONE_CONTAINER_CACHE_LOCK_STRIPS_DEFAULT); - cache = new ContainerCache(cacheSize, strips, LOAD_FACTOR, true); + int stripes = conf.getInt( + OzoneConfigKeys.OZONE_CONTAINER_CACHE_LOCK_STRIPES, + OzoneConfigKeys.OZONE_CONTAINER_CACHE_LOCK_STRIPES_DEFAULT); + cache = new ContainerCache(cacheSize, stripes, LOAD_FACTOR, true); } return cache; } From 2c9f550852446bc7d64bbac79c26f39bca7da61f Mon Sep 17 00:00:00 2001 From: Sammi Chen Date: Tue, 21 Jul 2020 20:43:01 +0800 Subject: [PATCH 11/11] add ozone.container.cache.lock.stripes in ozone-default.xml --- .../common/src/main/resources/ozone-default.xml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/hadoop-hdds/common/src/main/resources/ozone-default.xml b/hadoop-hdds/common/src/main/resources/ozone-default.xml index 3c5cd7c58f6f..3a6189df99d2 100644 --- a/hadoop-hdds/common/src/main/resources/ozone-default.xml +++ b/hadoop-hdds/common/src/main/resources/ozone-default.xml @@ -37,6 +37,16 @@ size of that cache. + + ozone.container.cache.lock.stripes + 1024 + PERFORMANCE, CONTAINER, STORAGE + Container DB open is an exclusive operation. We use a stripe + lock to guarantee that different threads can open different container DBs + concurrently, while for one container DB, only one thread can open it at + the same time. This setting controls the lock stripes. + + dfs.container.ipc 9859