From a0dc578c70dca2ba4b5025a097f4ac8aa9a29f30 Mon Sep 17 00:00:00 2001 From: Wei-Chiu Chuang Date: Mon, 13 Jun 2022 15:35:47 +0800 Subject: [PATCH 1/7] Fix potential leak Change-Id: Ie63955c03bc2202a352e3e74d8eafe3fdf74fae1 Conflicts: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java Change-Id: I7306e6f224a7ab466cd5b3222d949bd6531ba324 --- .../apache/hadoop/hdds/utils/db/RDBStore.java | 45 +-- .../hadoop/hdds/utils/db/RocksDatabase.java | 20 +- .../hadoop/ozone/om/KeyManagerImpl.java | 316 +++++++++--------- .../ozone/om/request/file/OMFileRequest.java | 33 +- .../apache/hadoop/ozone/debug/ListTables.java | 5 +- .../hadoop/ozone/debug/RocksDBUtils.java | 3 +- 6 files changed, 224 insertions(+), 198 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStore.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStore.java index baaa17e49ac7..e32987c17db3 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStore.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBStore.java @@ -283,9 +283,8 @@ public DBUpdatesWrapper getUpdatesSince(long sequenceNumber, long limitCount) throw new IllegalArgumentException("Illegal count for getUpdatesSince."); } DBUpdatesWrapper dbUpdatesWrapper = new DBUpdatesWrapper(); - try { - TransactionLogIterator transactionLogIterator = - db.getUpdatesSince(sequenceNumber); + try (TransactionLogIterator transactionLogIterator = + db.getUpdatesSince(sequenceNumber)) { // Only the first record needs to be checked if its seq number < // ( 1 + passed_in_sequence_number). For example, if seqNumber passed @@ -298,24 +297,28 @@ public DBUpdatesWrapper getUpdatesSince(long sequenceNumber, long limitCount) while (transactionLogIterator.isValid()) { TransactionLogIterator.BatchResult result = transactionLogIterator.getBatch(); - long currSequenceNumber = result.sequenceNumber(); - if (checkValidStartingSeqNumber && - currSequenceNumber > 1 + sequenceNumber) { - throw new SequenceNumberNotFoundException("Unable to read data from" + - " RocksDB wal to get delta updates. It may have already been" + - "flushed to SSTs."); - } - // If the above condition was not satisfied, then it is OK to reset - // the flag. - checkValidStartingSeqNumber = false; - if (currSequenceNumber <= sequenceNumber) { - transactionLogIterator.next(); - continue; - } - dbUpdatesWrapper.addWriteBatch(result.writeBatch().data(), - result.sequenceNumber()); - if (currSequenceNumber - sequenceNumber >= limitCount) { - break; + try { + long currSequenceNumber = result.sequenceNumber(); + if (checkValidStartingSeqNumber && + currSequenceNumber > 1 + sequenceNumber) { + throw new SequenceNumberNotFoundException("Unable to read data from" + + " RocksDB wal to get delta updates. It may have already been" + + "flushed to SSTs."); + } + // If the above condition was not satisfied, then it is OK to reset + // the flag. + checkValidStartingSeqNumber = false; + if (currSequenceNumber <= sequenceNumber) { + transactionLogIterator.next(); + continue; + } + dbUpdatesWrapper.addWriteBatch(result.writeBatch().data(), + result.sequenceNumber()); + if (currSequenceNumber - sequenceNumber >= limitCount) { + break; + } + } finally { + result.writeBatch().close(); } transactionLogIterator.next(); } diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java index 4ac83c912234..1289260259bc 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RocksDatabase.java @@ -73,12 +73,11 @@ static IOException toIOException(Object name, String op, RocksDBException e) { * Read DB and return existing column families. * * @return a list of column families. - * @see RocksDB#listColumnFamilies(Options, String) */ private static List getColumnFamilies(File file) throws RocksDBException { - final List columnFamilies = RocksDB.listColumnFamilies( - new Options(), file.getAbsolutePath()) + final List columnFamilies = listColumnFamiliesEmptyOptions( + file.getAbsolutePath()) .stream() .map(TableConfig::newTableConfig) .collect(Collectors.toList()); @@ -88,6 +87,21 @@ private static List getColumnFamilies(File file) return columnFamilies; } + /** + * Read DB column families without Options. + * @param path + * @return A list of column family names + * @throws RocksDBException + * + * @see RocksDB#listColumnFamilies(Options, String) + */ + public static List listColumnFamiliesEmptyOptions(final String path) + throws RocksDBException { + try (Options emptyOptions = new Options()) { + return RocksDB.listColumnFamilies(emptyOptions, path); + } + } + static RocksDatabase open(File dbFile, DBOptions dbOptions, WriteOptions writeOptions, Set families, boolean readOnly) throws IOException { diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java index 93c59a524a03..a4fcbd6c573a 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java @@ -1713,162 +1713,167 @@ public List listStatusFSO(OmKeyArgs args, boolean recursive, TreeMap tempCacheDirMap = new TreeMap<>(); TableIterator> - iterator; + iterator = null; - if (Strings.isNullOrEmpty(startKey)) { - OzoneFileStatus fileStatus = getFileStatus(args, clientAddress); - if (fileStatus.isFile()) { - return Collections.singletonList(fileStatus); - } + try { + if (Strings.isNullOrEmpty(startKey)) { + OzoneFileStatus fileStatus = getFileStatus(args, clientAddress); + if (fileStatus.isFile()) { + return Collections.singletonList(fileStatus); + } - // Not required to search in DeletedTable because all the deleted - // keys will be marked directly in dirTable or in keyTable by - // breaking the pointer to its sub-dirs and sub-files. So, there is no - // issue of inconsistency. + // Not required to search in DeletedTable because all the deleted + // keys will be marked directly in dirTable or in keyTable by + // breaking the pointer to its sub-dirs and sub-files. So, there is no + // issue of inconsistency. + + /* + * keyName is a directory. + * Say, "/a" is the dir name and its objectID is 1024, then seek + * will be doing with "1024/" to get all immediate descendants. + */ + if (fileStatus.getKeyInfo() != null) { + prefixKeyInDB = fileStatus.getKeyInfo().getObjectID(); + } else { + // list root directory. + prefixKeyInDB = bucketId; + } + seekFileInDB = metadataManager.getOzonePathKey( + volumeId, bucketId, prefixKeyInDB, ""); + seekDirInDB = metadataManager.getOzonePathKey( + volumeId, bucketId, prefixKeyInDB, ""); + + // Order of seek -> + // (1)Seek files in fileTable + // (2)Seek dirs in dirTable + + // First under lock obtain both entries from dir/file cache and generate + // entries marked for delete. + metadataManager.getLock() + .acquireReadLock(BUCKET_LOCK, volumeName, bucketName); + try { + BucketLayout bucketLayout = getBucketLayout(metadataManager, volumeName, bucketName); + iterator = metadataManager.getKeyTable(bucketLayout).iterator(); + countEntries = + getFilesAndDirsFromCacheWithBucket(volumeName, bucketName, + cacheFileMap, tempCacheDirMap, deletedKeySet, prefixKeyInDB, + seekFileInDB, seekDirInDB, prefixPath, startKey, countEntries, + numEntries); + + } finally { + metadataManager.getLock() + .releaseReadLock(BUCKET_LOCK, volumeName, bucketName); + } + countEntries = + getFilesFromDirectory(cacheFileMap, seekFileInDB, prefixPath, + prefixKeyInDB, countEntries, numEntries, deletedKeySet, iterator); - /* - * keyName is a directory. - * Say, "/a" is the dir name and its objectID is 1024, then seek - * will be doing with "1024/" to get all immediate descendants. - */ - if (fileStatus.getKeyInfo() != null) { - prefixKeyInDB = fileStatus.getKeyInfo().getObjectID(); } else { - // list root directory. - prefixKeyInDB = bucketId; - } - seekFileInDB = metadataManager.getOzonePathKey( - volumeId, bucketId, prefixKeyInDB, ""); - seekDirInDB = metadataManager.getOzonePathKey( - volumeId, bucketId, prefixKeyInDB, ""); - - // Order of seek -> - // (1)Seek files in fileTable - // (2)Seek dirs in dirTable - - - // First under lock obtain both entries from dir/file cache and generate - // entries marked for delete. - metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName, - bucketName); - try { - BucketLayout bucketLayout = - getBucketLayout(metadataManager, volumeName, bucketName); - iterator = metadataManager.getKeyTable(bucketLayout).iterator(); - countEntries = getFilesAndDirsFromCacheWithBucket(volumeName, - bucketName, cacheFileMap, tempCacheDirMap, deletedKeySet, - prefixKeyInDB, seekFileInDB, seekDirInDB, prefixPath, startKey, - countEntries, numEntries); - - } finally { - metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName, - bucketName); - } - countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB, - prefixPath, prefixKeyInDB, countEntries, numEntries, deletedKeySet, - iterator); - - } else { - /* - * startKey will be used in iterator seek and sets the beginning point - * for key traversal. - * keyName will be used as parentID where the user has requested to - * list the keys from. - * - * When recursive flag=false, parentID won't change between two pages. - * For example: OM has a namespace like, - * /a/1...1M files and /a/b/1...1M files. - * /a/1...1M directories and /a/b/1...1M directories. - * Listing "/a", will always have the parentID as "a" irrespective of - * the startKey value. - */ - - // Check startKey is an immediate child of keyName. For example, - // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b. - if (StringUtils.isNotBlank(keyName) && - !OzoneFSUtils.isImmediateChild(keyName, startKey)) { - if (LOG.isDebugEnabled()) { - LOG.debug("StartKey {} is not an immediate child of keyName {}. " + - "Returns empty list", startKey, keyName); + /* + * startKey will be used in iterator seek and sets the beginning point + * for key traversal. + * keyName will be used as parentID where the user has requested to + * list the keys from. + * + * When recursive flag=false, parentID won't change between two pages. + * For example: OM has a namespace like, + * /a/1...1M files and /a/b/1...1M files. + * /a/1...1M directories and /a/b/1...1M directories. + * Listing "/a", will always have the parentID as "a" irrespective of + * the startKey value. + */ + + // Check startKey is an immediate child of keyName. For example, + // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b. + if (StringUtils.isNotBlank(keyName) && !OzoneFSUtils + .isImmediateChild(keyName, startKey)) { + if (LOG.isDebugEnabled()) { + LOG.debug("StartKey {} is not an immediate child of keyName {}. " + + "Returns empty list", startKey, keyName); + } + return Collections.emptyList(); } - return Collections.emptyList(); - } - // assign startKeyPath if prefixPath is empty string. - if (StringUtils.isBlank(prefixPath)) { - prefixPath = OzoneFSUtils.getParentDir(startKey); - } + // assign startKeyPath if prefixPath is empty string. + if (StringUtils.isBlank(prefixPath)) { + prefixPath = OzoneFSUtils.getParentDir(startKey); + } - OmKeyArgs startKeyArgs = args.toBuilder() - .setKeyName(startKey) - .setSortDatanodesInPipeline(false) - .build(); - OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(startKeyArgs, - null, true); + OmKeyArgs startKeyArgs = args.toBuilder() + .setKeyName(startKey) + .setSortDatanodesInPipeline(false) + .build(); + OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(startKeyArgs, + null, true); - if (fileStatusInfo != null) { - prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID(); + if (fileStatusInfo != null) { + prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID(); - if (fileStatusInfo.isDirectory()) { - seekDirInDB = metadataManager.getOzonePathKey( - volumeId, bucketId, prefixKeyInDB, - fileStatusInfo.getKeyInfo().getFileName()); + if (fileStatusInfo.isDirectory()) { + seekDirInDB = metadataManager.getOzonePathKey( + volumeId, bucketId, prefixKeyInDB, + fileStatusInfo.getKeyInfo().getFileName()); - // Order of seek -> (1) Seek dirs only in dirTable. In OM, always - // the order of search is, first seek into fileTable and then - // dirTable. So, its not required to search again in the fileTable. + // Order of seek -> (1) Seek dirs only in dirTable. In OM, always + // the order of search is, first seek into fileTable and then + // dirTable. So, its not required to search again in the fileTable. - // Seek the given key in dirTable. - metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName, + // Seek the given key in dirTable. + metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName, bucketName); - try { - listStatusFindDirsInTableCache(tempCacheDirMap, - metadataManager.getDirectoryTable(), - prefixKeyInDB, seekDirInDB, prefixPath, startKey, volumeName, - bucketName, countEntries, numEntries, deletedKeySet); - } finally { - metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName, + try { + listStatusFindDirsInTableCache(tempCacheDirMap, + metadataManager.getDirectoryTable(), + prefixKeyInDB, seekDirInDB, prefixPath, startKey, volumeName, + bucketName, countEntries, numEntries, deletedKeySet); + } finally { + metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName, + bucketName); + } + + } else { + seekFileInDB = metadataManager.getOzonePathKey( + volumeId, bucketId, prefixKeyInDB, + fileStatusInfo.getKeyInfo().getFileName()); + // begins from the first sub-dir under the parent dir + seekDirInDB = metadataManager.getOzonePathKey( + volumeId, bucketId, prefixKeyInDB, ""); + + // First under lock obtain both entries from dir/file cache and + // generate entries marked for delete. + metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName, bucketName); - } + try { + BucketLayout bucketLayout = + getBucketLayout(metadataManager, volumeName, bucketName); + iterator = metadataManager.getKeyTable(bucketLayout) + .iterator(); + countEntries = getFilesAndDirsFromCacheWithBucket(volumeName, + bucketName, cacheFileMap, tempCacheDirMap, deletedKeySet, + prefixKeyInDB, seekFileInDB, seekDirInDB, prefixPath, startKey, + countEntries, numEntries); + } finally { + metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName, + bucketName); + } + // 1. Seek the given key in key table. + countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB, + prefixPath, prefixKeyInDB, countEntries, numEntries, + deletedKeySet, iterator); + } } else { - seekFileInDB = metadataManager.getOzonePathKey( - volumeId, bucketId, prefixKeyInDB, - fileStatusInfo.getKeyInfo().getFileName()); - // begins from the first sub-dir under the parent dir - seekDirInDB = metadataManager.getOzonePathKey( - volumeId, bucketId, prefixKeyInDB, ""); - - // First under lock obtain both entries from dir/file cache and - // generate entries marked for delete. - metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName, - bucketName); - try { - BucketLayout bucketLayout = - getBucketLayout(metadataManager, volumeName, bucketName); - iterator = metadataManager.getKeyTable(bucketLayout) - .iterator(); - countEntries = getFilesAndDirsFromCacheWithBucket(volumeName, - bucketName, cacheFileMap, tempCacheDirMap, deletedKeySet, - prefixKeyInDB, seekFileInDB, seekDirInDB, prefixPath, startKey, - countEntries, numEntries); - } finally { - metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName, - bucketName); + // TODO: HDDS-4364: startKey can be a non-existed key + if (LOG.isDebugEnabled()) { + LOG.debug("StartKey {} is a non-existed key and returning empty " + + "list", startKey); } - - // 1. Seek the given key in key table. - countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB, - prefixPath, prefixKeyInDB, countEntries, numEntries, - deletedKeySet, iterator); + return Collections.emptyList(); } - } else { - // TODO: HDDS-4364: startKey can be a non-existed key - if (LOG.isDebugEnabled()) { - LOG.debug("StartKey {} is a non-existed key and returning empty " + - "list", startKey); - } - return Collections.emptyList(); + } + } finally { + if (iterator != null) { + iterator.close(); } } @@ -2421,25 +2426,26 @@ public List getPendingDeletionSubFiles(long volumeId, long countEntries = 0; Table fileTable = metadataManager.getFileTable(); - TableIterator> - iterator = fileTable.iterator(); + try (TableIterator> + iterator = fileTable.iterator()) { - iterator.seek(seekFileInDB); + iterator.seek(seekFileInDB); - while (iterator.hasNext() && numEntries - countEntries > 0) { - Table.KeyValue entry = iterator.next(); - OmKeyInfo fileInfo = entry.getValue(); - if (!OMFileRequest.isImmediateChild(fileInfo.getParentObjectID(), - parentInfo.getObjectID())) { - break; - } - fileInfo.setFileName(fileInfo.getKeyName()); - String fullKeyPath = OMFileRequest.getAbsolutePath( - parentInfo.getKeyName(), fileInfo.getKeyName()); - fileInfo.setKeyName(fullKeyPath); + while (iterator.hasNext() && numEntries - countEntries > 0) { + Table.KeyValue entry = iterator.next(); + OmKeyInfo fileInfo = entry.getValue(); + if (!OMFileRequest.isImmediateChild(fileInfo.getParentObjectID(), + parentInfo.getObjectID())) { + break; + } + fileInfo.setFileName(fileInfo.getKeyName()); + String fullKeyPath =OMFileRequest.getAbsolutePath( + parentInfo.getKeyName(), fileInfo.getKeyName()); + fileInfo.setKeyName(fullKeyPath); - files.add(fileInfo); - countEntries++; + files.add(fileInfo); + countEntries++; + } } return files; diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java index 8363be8d9bcf..496e843dda17 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java @@ -902,16 +902,17 @@ private static boolean checkSubDirectoryExists(OmKeyInfo omKeyInfo, // Check dirTable entries for any sub paths. String seekDirInDB = metaMgr.getOzonePathKey(volumeId, bucketId, omKeyInfo.getObjectID(), ""); - TableIterator> - iterator = dirTable.iterator(); + try (TableIterator> + iterator = dirTable.iterator()) { - iterator.seek(seekDirInDB); + iterator.seek(seekDirInDB); + + if (iterator.hasNext()) { + Table.KeyValue entry = iterator.next(); + OmDirectoryInfo dirInfo = entry.getValue(); + return isImmediateChild(dirInfo.getParentObjectID(), omKeyInfo.getObjectID()); + } - if (iterator.hasNext()) { - Table.KeyValue entry = iterator.next(); - OmDirectoryInfo dirInfo = entry.getValue(); - return isImmediateChild(dirInfo.getParentObjectID(), - omKeyInfo.getObjectID()); } return false; // no sub paths found } @@ -946,16 +947,16 @@ private static boolean checkSubFileExists(OmKeyInfo omKeyInfo, // Check fileTable entries for any sub paths. String seekFileInDB = metaMgr.getOzonePathKey(volumeId, bucketId, omKeyInfo.getObjectID(), ""); - TableIterator> - iterator = fileTable.iterator(); + try (TableIterator> + iterator = fileTable.iterator()) { - iterator.seek(seekFileInDB); + iterator.seek(seekFileInDB); - if (iterator.hasNext()) { - Table.KeyValue entry = iterator.next(); - OmKeyInfo fileInfo = entry.getValue(); - return isImmediateChild(fileInfo.getParentObjectID(), - omKeyInfo.getObjectID()); // found a sub path file + if (iterator.hasNext()) { + Table.KeyValue entry = iterator.next(); + OmKeyInfo fileInfo = entry.getValue(); + return isImmediateChild(fileInfo.getParentObjectID(), omKeyInfo.getObjectID()); // found a sub path file + } } return false; // no sub paths found } diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ListTables.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ListTables.java index be1cd592d7cf..28b263820a66 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ListTables.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ListTables.java @@ -24,6 +24,7 @@ import org.apache.hadoop.hdds.cli.SubcommandWithParent; +import org.apache.hadoop.hdds.utils.db.RocksDatabase; import org.kohsuke.MetaInfServices; import org.rocksdb.Options; import org.rocksdb.RocksDB; @@ -45,8 +46,8 @@ public class ListTables implements Callable, SubcommandWithParent { @Override public Void call() throws Exception { - List columnFamilies = RocksDB.listColumnFamilies(new Options(), - parent.getDbPath()); + List columnFamilies = RocksDatabase.listColumnFamiliesEmptyOptions( + parent.getDbPath()); for (byte[] b : columnFamilies) { System.out.println(new String(b, StandardCharsets.UTF_8)); } diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/RocksDBUtils.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/RocksDBUtils.java index 24f6e21833f1..8964a6ae62a5 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/RocksDBUtils.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/RocksDBUtils.java @@ -18,6 +18,7 @@ package org.apache.hadoop.ozone.debug; +import org.apache.hadoop.hdds.utils.db.RocksDatabase; import org.rocksdb.ColumnFamilyDescriptor; import org.rocksdb.Options; import org.rocksdb.RocksDB; @@ -38,7 +39,7 @@ private RocksDBUtils() { public static List getColumnFamilyDescriptors( String dbPath) throws RocksDBException { List cfs = new ArrayList<>(); - List cfList = RocksDB.listColumnFamilies(new Options(), dbPath); + List cfList = RocksDatabase.listColumnFamiliesEmptyOptions(dbPath); if (cfList != null) { for (byte[] b : cfList) { cfs.add(new ColumnFamilyDescriptor(b)); From 8f576f2684f3c13dca716b6f52128b564e4c6e3a Mon Sep 17 00:00:00 2001 From: Wei-Chiu Chuang Date: Thu, 16 Jun 2022 01:15:38 +0800 Subject: [PATCH 2/7] checkstyle Change-Id: I7bbd272ed5c7ed2fd1328a4189404de9116c699a --- .../main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java index a4fcbd6c573a..57463f791245 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java @@ -2439,7 +2439,7 @@ public List getPendingDeletionSubFiles(long volumeId, break; } fileInfo.setFileName(fileInfo.getKeyName()); - String fullKeyPath =OMFileRequest.getAbsolutePath( + String fullKeyPath = OMFileRequest.getAbsolutePath( parentInfo.getKeyName(), fileInfo.getKeyName()); fileInfo.setKeyName(fullKeyPath); From a89da04bcc1b34625d3a1f376866ee4cbb3b3ac3 Mon Sep 17 00:00:00 2001 From: Wei-Chiu Chuang Date: Fri, 17 Jun 2022 03:20:10 +0800 Subject: [PATCH 3/7] Ensure org.rocksdb.Options are properly closed. Change-Id: Ie4f2aab3f6531da2cb093f8e8086afc483072d1f --- .../apache/hadoop/hdds/utils/db/RDBSstFileWriter.java | 4 +++- .../org/apache/hadoop/ozone/om/KeyManagerImpl.java | 10 ++++++---- .../hadoop/ozone/om/request/file/OMFileRequest.java | 9 ++++++--- .../java/org/apache/hadoop/ozone/debug/ListTables.java | 2 -- .../org/apache/hadoop/ozone/debug/RocksDBUtils.java | 2 -- 5 files changed, 15 insertions(+), 12 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBSstFileWriter.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBSstFileWriter.java index 367c235f734b..d440df174841 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBSstFileWriter.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBSstFileWriter.java @@ -37,10 +37,11 @@ public class RDBSstFileWriter implements DumpFileWriter, Closeable { private SstFileWriter sstFileWriter; private File sstFile; private AtomicLong keyCounter; + private Options emptyOption = new Options(); public RDBSstFileWriter() { EnvOptions envOptions = new EnvOptions(); - this.sstFileWriter = new SstFileWriter(envOptions, new Options()); + this.sstFileWriter = new SstFileWriter(envOptions, emptyOption); this.keyCounter = new AtomicLong(0); } @@ -83,6 +84,7 @@ public void close() throws IOException { } finally { sstFileWriter.close(); sstFileWriter = null; + emptyOption.close(); } keyCounter.set(0); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java index 57463f791245..05af4e7e5d51 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java @@ -1752,7 +1752,8 @@ public List listStatusFSO(OmKeyArgs args, boolean recursive, metadataManager.getLock() .acquireReadLock(BUCKET_LOCK, volumeName, bucketName); try { - BucketLayout bucketLayout = getBucketLayout(metadataManager, volumeName, bucketName); + BucketLayout bucketLayout = getBucketLayout( + metadataManager, volumeName, bucketName); iterator = metadataManager.getKeyTable(bucketLayout).iterator(); countEntries = getFilesAndDirsFromCacheWithBucket(volumeName, bucketName, @@ -1766,7 +1767,8 @@ public List listStatusFSO(OmKeyArgs args, boolean recursive, } countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB, prefixPath, - prefixKeyInDB, countEntries, numEntries, deletedKeySet, iterator); + prefixKeyInDB, countEntries, numEntries, deletedKeySet, + iterator); } else { /* @@ -1850,8 +1852,8 @@ public List listStatusFSO(OmKeyArgs args, boolean recursive, .iterator(); countEntries = getFilesAndDirsFromCacheWithBucket(volumeName, bucketName, cacheFileMap, tempCacheDirMap, deletedKeySet, - prefixKeyInDB, seekFileInDB, seekDirInDB, prefixPath, startKey, - countEntries, numEntries); + prefixKeyInDB, seekFileInDB, seekDirInDB, prefixPath, + startKey, countEntries, numEntries); } finally { metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName, bucketName); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java index 496e843dda17..73902099021c 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java @@ -902,7 +902,8 @@ private static boolean checkSubDirectoryExists(OmKeyInfo omKeyInfo, // Check dirTable entries for any sub paths. String seekDirInDB = metaMgr.getOzonePathKey(volumeId, bucketId, omKeyInfo.getObjectID(), ""); - try (TableIterator> + try (TableIterator> iterator = dirTable.iterator()) { iterator.seek(seekDirInDB); @@ -910,7 +911,8 @@ private static boolean checkSubDirectoryExists(OmKeyInfo omKeyInfo, if (iterator.hasNext()) { Table.KeyValue entry = iterator.next(); OmDirectoryInfo dirInfo = entry.getValue(); - return isImmediateChild(dirInfo.getParentObjectID(), omKeyInfo.getObjectID()); + return isImmediateChild(dirInfo.getParentObjectID(), + omKeyInfo.getObjectID()); } } @@ -955,7 +957,8 @@ private static boolean checkSubFileExists(OmKeyInfo omKeyInfo, if (iterator.hasNext()) { Table.KeyValue entry = iterator.next(); OmKeyInfo fileInfo = entry.getValue(); - return isImmediateChild(fileInfo.getParentObjectID(), omKeyInfo.getObjectID()); // found a sub path file + return isImmediateChild(fileInfo.getParentObjectID(), + omKeyInfo.getObjectID()); // found a sub path file } } return false; // no sub paths found diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ListTables.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ListTables.java index 28b263820a66..494f42e5877e 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ListTables.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/ListTables.java @@ -26,8 +26,6 @@ import org.apache.hadoop.hdds.utils.db.RocksDatabase; import org.kohsuke.MetaInfServices; -import org.rocksdb.Options; -import org.rocksdb.RocksDB; import picocli.CommandLine; /** diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/RocksDBUtils.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/RocksDBUtils.java index 8964a6ae62a5..cb780da450bb 100644 --- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/RocksDBUtils.java +++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/RocksDBUtils.java @@ -20,8 +20,6 @@ import org.apache.hadoop.hdds.utils.db.RocksDatabase; import org.rocksdb.ColumnFamilyDescriptor; -import org.rocksdb.Options; -import org.rocksdb.RocksDB; import org.rocksdb.RocksDBException; import java.util.ArrayList; From fa7a698cbb5e29be42cbf57118756dba05ab65dc Mon Sep 17 00:00:00 2001 From: Wei-Chiu Chuang Date: Mon, 27 Jun 2022 10:54:48 -0700 Subject: [PATCH 4/7] Close TableIterator in getPendingDeletionSubDirs. Change-Id: I6088fc7aa1e967d6d59e3ef1c6a697c1d0ae5b39 --- .../apache/hadoop/ozone/om/KeyManagerImpl.java | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java index 05af4e7e5d51..0825367f4593 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java @@ -2388,15 +2388,27 @@ public Table.KeyValue getPendingDeletionDir() @Override public List getPendingDeletionSubDirs(long volumeId, long bucketId, OmKeyInfo parentInfo, long numEntries) throws IOException { - List directories = new ArrayList<>(); String seekDirInDB = metadataManager.getOzonePathKey(volumeId, bucketId, parentInfo.getObjectID(), ""); long countEntries = 0; Table dirTable = metadataManager.getDirectoryTable(); - TableIterator> - iterator = dirTable.iterator(); + try (TableIterator> + iterator = dirTable.iterator()) { + return gatherSubDirsWithIterator(parentInfo, numEntries, + seekDirInDB, countEntries, iterator); + } + } + + private List gatherSubDirsWithIterator(OmKeyInfo parentInfo, + long numEntries, String seekDirInDB, + long countEntries, + TableIterator> iterator) + throws IOException { + List directories = new ArrayList<>(); iterator.seek(seekDirInDB); while (iterator.hasNext() && numEntries - countEntries > 0) { From c3ac16e76f89fcec4db3aad7035b6ec2de0c660a Mon Sep 17 00:00:00 2001 From: Wei-Chiu Chuang Date: Mon, 27 Jun 2022 10:58:04 -0700 Subject: [PATCH 5/7] Close TableIterator in getDirectories. Change-Id: I99fb164c9e2137db6f52cdcd556a9919222a0252 --- .../hadoop/ozone/om/KeyManagerImpl.java | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java index 0825367f4593..83e2f1fa2da9 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java @@ -2007,9 +2007,25 @@ protected int getDirectories( throws IOException { Table dirTable = metadataManager.getDirectoryTable(); - TableIterator> - iterator = dirTable.iterator(); + try (TableIterator> + iterator = dirTable.iterator()) { + + return getDirectoriesWithIterator(cacheKeyMap, seekDirInDB, prefixPath, + prefixKeyInDB, countEntries, numEntries, recursive, volumeName, + bucketName, deletedKeySet, iterator); + } + } + @SuppressWarnings("parameternumber") + private int getDirectoriesWithIterator( + TreeMap cacheKeyMap, String seekDirInDB, + String prefixPath, long prefixKeyInDB, int countEntries, long numEntries, + boolean recursive, String volumeName, String bucketName, + Set deletedKeySet, + TableIterator> iterator) + throws IOException { iterator.seek(seekDirInDB); while (iterator.hasNext() && numEntries - countEntries > 0) { From d4abbb951f34dbf2654161b1d3afa64fb5d8225b Mon Sep 17 00:00:00 2001 From: Wei-Chiu Chuang Date: Mon, 27 Jun 2022 11:30:20 -0700 Subject: [PATCH 6/7] Close TableIterator in listStatus. Change-Id: I0d2a5bf89d56faafaa7948fbeb98db768ab1ada8 --- .../hadoop/ozone/om/KeyManagerImpl.java | 77 ++++++++++++------- 1 file changed, 49 insertions(+), 28 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java index 83e2f1fa2da9..2423e9923534 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java @@ -1550,8 +1550,46 @@ public List listStatus(OmKeyArgs args, boolean recursive, bucketName); Table keyTable = metadataManager .getKeyTable(getBucketLayout(metadataManager, volName, buckName)); - TableIterator> - iterator; + try(TableIterator> + iterator = getIteratorForKeyInTableCache(recursive, startKey, + volumeName, bucketName, cacheKeyMap, keyArgs, keyTable)) { + findKeyInDbWithIterator(recursive, startKey, numEntries, volumeName, + bucketName, keyName, cacheKeyMap, keyArgs, keyTable, iterator); + } + int countEntries; + + countEntries = 0; + // Convert results in cacheKeyMap to List + for (OzoneFileStatus fileStatus : cacheKeyMap.values()) { + // No need to check if a key is deleted or not here, this is handled + // when adding entries to cacheKeyMap from DB. + fileStatusList.add(fileStatus); + countEntries++; + if (countEntries >= numEntries) { + break; + } + } + // Clean up temp map and set + cacheKeyMap.clear(); + + List keyInfoList = new ArrayList<>(fileStatusList.size()); + fileStatusList.stream().map(s -> s.getKeyInfo()).forEach(keyInfoList::add); + if (args.getLatestVersionLocation()) { + slimLocationVersion(keyInfoList.toArray(new OmKeyInfo[0])); + } + refreshPipeline(keyInfoList); + + if (args.getSortDatanodes()) { + sortDatanodes(clientAddress, keyInfoList.toArray(new OmKeyInfo[0])); + } + return fileStatusList; + } + + private TableIterator> getIteratorForKeyInTableCache( + boolean recursive, String startKey, String volumeName, String bucketName, + TreeMap cacheKeyMap, String keyArgs, + Table keyTable) { + TableIterator> iterator; try { Iterator, CacheValue>> cacheIter = keyTable.cacheIterator(); @@ -1567,7 +1605,16 @@ public List listStatus(OmKeyArgs args, boolean recursive, metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName, bucketName); } + return iterator; + } + private void findKeyInDbWithIterator(boolean recursive, String startKey, + long numEntries, String volumeName, String bucketName, String keyName, + TreeMap cacheKeyMap, String keyArgs, + Table keyTable, + TableIterator> iterator) + throws IOException { // Then, find key in DB String seekKeyInDb = metadataManager.getOzoneKey(volumeName, bucketName, startKey); @@ -1634,32 +1681,6 @@ public List listStatus(OmKeyArgs args, boolean recursive, } } } - - countEntries = 0; - // Convert results in cacheKeyMap to List - for (OzoneFileStatus fileStatus : cacheKeyMap.values()) { - // No need to check if a key is deleted or not here, this is handled - // when adding entries to cacheKeyMap from DB. - fileStatusList.add(fileStatus); - countEntries++; - if (countEntries >= numEntries) { - break; - } - } - // Clean up temp map and set - cacheKeyMap.clear(); - - List keyInfoList = new ArrayList<>(fileStatusList.size()); - fileStatusList.stream().map(s -> s.getKeyInfo()).forEach(keyInfoList::add); - if (args.getLatestVersionLocation()) { - slimLocationVersion(keyInfoList.toArray(new OmKeyInfo[0])); - } - refreshPipeline(keyInfoList); - - if (args.getSortDatanodes()) { - sortDatanodes(clientAddress, keyInfoList.toArray(new OmKeyInfo[0])); - } - return fileStatusList; } @SuppressWarnings("methodlength") From b1561efb1389cb9065021662fa1b8754d0aee74b Mon Sep 17 00:00:00 2001 From: Wei-Chiu Chuang Date: Mon, 27 Jun 2022 13:56:08 -0700 Subject: [PATCH 7/7] Checkstyle Change-Id: I4e477d9ad8eeeda6ecf71f610eb657f394740cfd --- .../java/org/apache/hadoop/ozone/om/KeyManagerImpl.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java index 2423e9923534..bd47ac843866 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java @@ -1550,7 +1550,7 @@ public List listStatus(OmKeyArgs args, boolean recursive, bucketName); Table keyTable = metadataManager .getKeyTable(getBucketLayout(metadataManager, volName, buckName)); - try(TableIterator> + try (TableIterator> iterator = getIteratorForKeyInTableCache(recursive, startKey, volumeName, bucketName, cacheKeyMap, keyArgs, keyTable)) { findKeyInDbWithIterator(recursive, startKey, numEntries, volumeName, @@ -1585,7 +1585,8 @@ public List listStatus(OmKeyArgs args, boolean recursive, return fileStatusList; } - private TableIterator> getIteratorForKeyInTableCache( + private TableIterator> + getIteratorForKeyInTableCache( boolean recursive, String startKey, String volumeName, String bucketName, TreeMap cacheKeyMap, String keyArgs, Table keyTable) { @@ -1608,6 +1609,7 @@ public List listStatus(OmKeyArgs args, boolean recursive, return iterator; } + @SuppressWarnings("parameternumber") private void findKeyInDbWithIterator(boolean recursive, String startKey, long numEntries, String volumeName, String bucketName, String keyName, TreeMap cacheKeyMap, String keyArgs,