This is an automated email from the ASF dual-hosted git repository. ckj pushed a commit to branch ozone-1.3 in repository https://gitbox.apache.org/repos/asf/ozone.git
commit acd7b229857b17109c638553db39fd04e22a538a Author: Kaijie Chen <[email protected]> AuthorDate: Wed Feb 8 23:31:23 2023 +0800 HDDS-7871. Fix false positive in KeyManagerImpl#createFakeDirIfShould() (#4236) --- .../apache/hadoop/ozone/om/TestKeyManagerImpl.java | 113 +++++++++++++++++++++ .../org/apache/hadoop/ozone/om/KeyManagerImpl.java | 23 ++--- 2 files changed, 123 insertions(+), 13 deletions(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java index 7a155d3f7d..9a4b5c315f 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java @@ -33,7 +33,9 @@ import java.util.Map; import java.util.Set; import java.util.TreeSet; import java.util.UUID; +import java.util.stream.Stream; +import org.apache.commons.lang3.tuple.Pair; import org.apache.hadoop.conf.StorageUnit; import org.apache.hadoop.hdds.HddsConfigKeys; import org.apache.hadoop.hdds.client.BlockID; @@ -120,6 +122,9 @@ import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Timeout; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.mockito.Mockito; @@ -158,6 +163,7 @@ public class TestKeyManagerImpl { private static long scmBlockSize; private static final String KEY_NAME = "key1"; private static final String BUCKET_NAME = "bucket1"; + private static final String BUCKET2_NAME = "bucket2"; private static final String VERSIONED_BUCKET_NAME = "versionedBucket1"; private static final String VOLUME_NAME = "vol1"; private static OzoneManagerProtocol writeClient; @@ -218,6 +224,7 @@ public class TestKeyManagerImpl { ResultCodes.SAFE_MODE_EXCEPTION)); createVolume(VOLUME_NAME); createBucket(VOLUME_NAME, BUCKET_NAME, false); + createBucket(VOLUME_NAME, BUCKET2_NAME, false); createBucket(VOLUME_NAME, VERSIONED_BUCKET_NAME, true); } @@ -1344,6 +1351,67 @@ public class TestKeyManagerImpl { assertTrue(ozoneFileStatus.isFile()); } + private static Stream<Arguments> fakeDirScenarios() { + final String bucket1 = BUCKET_NAME; + final String bucket2 = BUCKET2_NAME; + + return Stream.of( + Arguments.of( + "false positive", + Stream.of( + Pair.of(bucket1, "dir1/file1"), + Pair.of(bucket2, "dir2/file2") + ), + // positives + Stream.of( + Pair.of(bucket1, "dir1"), + Pair.of(bucket2, "dir2") + ), + // negatives + Stream.of( + Pair.of(bucket1, "dir0"), + // RocksIterator#seek("volume1/bucket1/dir2/") will position + // at the 2nd dbKey "volume1/bucket2/dir2/file2", which is + // not belong to bucket1. + // This might be a false positive, see HDDS-7871. + Pair.of(bucket1, "dir2"), + Pair.of(bucket2, "dir0"), + Pair.of(bucket2, "dir1"), + Pair.of(bucket2, "dir3") + ) + ), + Arguments.of( + "false negative", + Stream.of( + Pair.of(bucket1, "dir1/file1"), + Pair.of(bucket1, "dir1/file2"), + Pair.of(bucket1, "dir1/file3"), + Pair.of(bucket2, "dir1/file1"), + Pair.of(bucket2, "dir1/file2") + ), + // positives + Stream.of( + Pair.of(bucket1, "dir1"), + Pair.of(bucket2, "dir1") + ), + // negatives + Stream.empty() + ) + ); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("fakeDirScenarios") + public void testGetFileStatusWithFakeDirs( + String description, + Stream<Pair<String, String>> keys, + Stream<Pair<String, String>> positives, + Stream<Pair<String, String>> negatives) { + keys.forEach(f -> createFile(f.getLeft(), f.getRight())); + positives.forEach(f -> assertIsDirectory(f.getLeft(), f.getRight())); + negatives.forEach(f -> assertFileNotFound(f.getLeft(), f.getRight())); + } + @Test public void testRefreshPipeline() throws Exception { @@ -1578,6 +1646,51 @@ public class TestKeyManagerImpl { return keyNames; } + private void createFile(String bucketName, String keyName) { + try { + OmKeyArgs keyArgs = createBuilder(bucketName).setKeyName(keyName).build(); + OpenKeySession keySession = writeClient.openKey(keyArgs); + keyArgs.setLocationInfoList(keySession.getKeyInfo() + .getLatestVersionLocations().getLocationList()); + writeClient.commitKey(keyArgs, keySession.getId()); + + // verify key exist in table + OmKeyInfo keyInfo = metadataManager.getKeyTable(getDefaultBucketLayout()) + .get(metadataManager.getOzoneKey(VOLUME_NAME, bucketName, keyName)); + assertNotNull(keyInfo); + assertEquals(VOLUME_NAME, keyInfo.getVolumeName()); + assertEquals(bucketName, keyInfo.getBucketName()); + assertEquals(keyName, keyInfo.getKeyName()); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + private void assertFileNotFound(String bucketName, String keyName) { + try { + OmKeyArgs keyArgs = createBuilder(bucketName).setKeyName(keyName).build(); + OMException ex = assertThrows(OMException.class, + () -> keyManager.getFileStatus(keyArgs)); + assertEquals(OMException.ResultCodes.FILE_NOT_FOUND, ex.getResult()); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + private void assertIsDirectory(String bucketName, String keyName) { + try { + OmKeyArgs keyArgs = createBuilder(bucketName).setKeyName(keyName).build(); + OzoneFileStatus ozoneFileStatus = keyManager.getFileStatus(keyArgs); + OmKeyInfo keyInfo = ozoneFileStatus.getKeyInfo(); + assertEquals(VOLUME_NAME, keyInfo.getVolumeName()); + assertEquals(bucketName, keyInfo.getBucketName()); + assertEquals(keyName, keyInfo.getFileName()); + assertTrue(ozoneFileStatus.isDirectory()); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + private OmKeyArgs.Builder createBuilder() throws IOException { return createBuilder(BUCKET_NAME); } 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 8bf672fa7d..51663a0ac1 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 @@ -1298,21 +1298,18 @@ public class KeyManagerImpl implements KeyManager { String keyName, BucketLayout layout) throws IOException { OmKeyInfo fakeDirKeyInfo = null; String dirKey = OzoneFSUtils.addTrailingSlashIfNeeded(keyName); - String fileKeyBytes = metadataManager.getOzoneKey(volume, bucket, keyName); + String targetKey = OzoneFSUtils.addTrailingSlashIfNeeded( + metadataManager.getOzoneKey(volume, bucket, keyName)); try (TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>> keyTblItr = metadataManager.getKeyTable(layout).iterator()) { - Table.KeyValue<String, OmKeyInfo> keyValue = - keyTblItr - .seek(OzoneFSUtils.addTrailingSlashIfNeeded(fileKeyBytes)); - - if (keyValue != null) { - Path fullPath = Paths.get(keyValue.getValue().getKeyName()); - Path subPath = Paths.get(dirKey); - OmKeyInfo omKeyInfo = keyValue.getValue(); - if (fullPath.startsWith(subPath)) { - // create fake directory - fakeDirKeyInfo = createDirectoryKey(omKeyInfo, dirKey); - } + Table.KeyValue<String, OmKeyInfo> keyValue = keyTblItr.seek(targetKey); + + // HDDS-7871: RocksIterator#seek() may position at the key + // past the target, we should check the full dbKeyName. + // For example, seeking "/vol1/bucket1/dir2/" may return a key + // in different volume/bucket, such as "/vol1/bucket2/dir2/key2". + if (keyValue != null && keyValue.getKey().startsWith(targetKey)) { + fakeDirKeyInfo = createDirectoryKey(keyValue.getValue(), dirKey); } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
