From 203c3a1aafbd2e7ae684e73d451b1d84db2f9e81 Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Tue, 18 Feb 2025 12:36:40 -0800 Subject: [PATCH 1/3] HDDS-12368. table iterator in KeyManagerImpl should seek to correct start key Change-Id: I6597338be1826c052daa7601996fb576f8f657c1 --- .../src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java | 1 - 1 file changed, 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 54b4608e64f2..43106c06eb56 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 @@ -696,7 +696,6 @@ private List> getTableEntries(String startKey, */ if (startKey != null) { tableIterator.seek(startKey); - tableIterator.seekToFirst(); } int currentCount = 0; while (tableIterator.hasNext() && currentCount < size) { From 1150c299c80c625d4786a7ba9bf9088ab4aa3f6a Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Mon, 31 Mar 2025 09:06:30 -0700 Subject: [PATCH 2/3] HDDS-12368. Add test cases Change-Id: I3a5d9c1ab7a7e4f4639ceaf14f434c3e45e32f1b --- .../hdds/utils/MapBackedTableIterator.java | 83 +++++++ hadoop-ozone/ozone-manager/pom.xml | 7 + .../hadoop/ozone/om/KeyManagerImpl.java | 2 + .../hadoop/ozone/om/TestKeyManagerImpl.java | 214 ++++++++++++++++++ 4 files changed, 306 insertions(+) create mode 100644 hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/MapBackedTableIterator.java create mode 100644 hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/MapBackedTableIterator.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/MapBackedTableIterator.java new file mode 100644 index 000000000000..0ca7b452512e --- /dev/null +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/utils/MapBackedTableIterator.java @@ -0,0 +1,83 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hdds.utils; + +import java.io.IOException; +import java.util.Iterator; +import java.util.Map; +import java.util.TreeMap; +import org.apache.hadoop.hdds.utils.db.Table; +import org.apache.hadoop.hdds.utils.db.TableIterator; + +/** + * Generic Table Iterator implementation that can be used for unit tests to reduce redundant mocking in tests. + */ +public class MapBackedTableIterator implements TableIterator> { + + private Iterator> itr; + private final String prefix; + private final TreeMap values; + + public MapBackedTableIterator(TreeMap values, String prefix) { + this.prefix = prefix; + this.values = values; + this.seekToFirst(); + } + + @Override + public void seekToFirst() { + this.itr = this.values.entrySet().stream() + .filter(e -> prefix == null || e.getKey().startsWith(prefix)) + .map(e -> Table.newKeyValue(e.getKey(), e.getValue())).iterator(); + } + + @Override + public void seekToLast() { + + } + + @Override + public Table.KeyValue seek(String s) throws IOException { + this.itr = this.values.entrySet().stream() + .filter(e -> prefix == null || e.getKey().startsWith(prefix)) + .filter(e -> e.getKey().compareTo(s) >= 0) + .map(e -> Table.newKeyValue(e.getKey(), e.getValue())).iterator(); + Map.Entry firstEntry = values.ceilingEntry(s); + return firstEntry == null ? null : Table.newKeyValue(firstEntry.getKey(), firstEntry.getValue()); + } + + @Override + public void removeFromDB() throws IOException { + + } + + @Override + public void close() throws IOException { + + } + + @Override + public boolean hasNext() { + return this.itr.hasNext(); + } + + @Override + public Table.KeyValue next() { + return itr.next(); + } +} diff --git a/hadoop-ozone/ozone-manager/pom.xml b/hadoop-ozone/ozone-manager/pom.xml index 9e5fade5f4c0..d75b9f1de633 100644 --- a/hadoop-ozone/ozone-manager/pom.xml +++ b/hadoop-ozone/ozone-manager/pom.xml @@ -362,6 +362,13 @@ test-jar test + + org.apache.ozone + hdds-server-framework + ${hdds.version} + test-jar + test + org.apache.ozone hdds-server-scm 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 43106c06eb56..9ed917329e6d 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 @@ -696,6 +696,8 @@ private List> getTableEntries(String startKey, */ if (startKey != null) { tableIterator.seek(startKey); + } else { + tableIterator.seekToFirst(); } int currentCount = 0; while (tableIterator.hasNext() && currentCount < size) { diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java new file mode 100644 index 000000000000..22740426e29c --- /dev/null +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestKeyManagerImpl.java @@ -0,0 +1,214 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.om; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.when; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.TreeMap; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.utils.MapBackedTableIterator; +import org.apache.hadoop.hdds.utils.db.Table; +import org.apache.hadoop.ozone.om.helpers.OmKeyInfo; +import org.apache.hadoop.ozone.om.helpers.RepeatedOmKeyInfo; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.Mockito; + +/** + * Test class for unit tests KeyManagerImpl. + */ +public class TestKeyManagerImpl { + private static Stream getTableIteratorParameters() { + return Stream.of( + Arguments.argumentSet("Fetch first 50 entries for volume 0, bucket 0", + 5, 10, 100, 0, 0, 0, 0, 0, 50, null), + Arguments.argumentSet("Fetch first 50 entries for any volume/bucket", 5, 10, 100, null, null, 0, 0, 0, 50, + null), + Arguments.argumentSet("Fetch first 30 entries for volume 1, bucket 1", 5, 10, 100, 1, 1, 0, 0, 0, 30, null), + Arguments.argumentSet("Fetch 20 entries from offset (2,2,10) for volume 2, bucket 2", 5, 10, 100, 2, 2, 2, 2, + 10, 20, null), + Arguments.argumentSet("Fetch 40 entries from offset (2,2,50) for volume 3, bucket 3", 5, 10, 100, 3, 3, 3, 3, + 50, 40, null), + Arguments.argumentSet("Fetch 200 entries from the very beginning (null start offsets)", 5, 10, 100, null, + null, null, null, null, 200, null), + Arguments.argumentSet("Fetch 200 entries starting from bucket 3, key 50, spanning 3 buckets", 5, 10, 100, + null, null, 0, 3, 50, 200, null), + Arguments.argumentSet("Invalid: bucket is set but volume is null", 5, 10, 100, null, 1, 0, 0, 0, 10, + IOException.class), + Arguments.argumentSet("Invalid: volume is set but bucket is null", 5, 10, 100, 1, null, 0, 0, 0, 10, + IOException.class), + Arguments.argumentSet("Fetch 50 entries from volume 2, bucket 5, but only 31 exist", 5, 10, 100, 2, 5, 2, 5, + 70, 50, null), + Arguments.argumentSet("Start from last volume (4), second-last bucket (8), key 80 but only 131 entries exist", + 5, 10, 100, null, null, 4, 8, 80, 200, null) + ); + } + + @SuppressWarnings({"checkstyle:ParameterNumber"}) + private List> mockTableIterator( + Class valueClass, Table table, int numberOfVolumes, int numberOfBucketsPerVolume, + int numberOfKeysPerBucket, String volumeNamePrefix, String bucketNamePrefix, String keyPrefix, + Integer volumeNumberFilter, Integer bucketNumberFilter, Integer startVolumeNumber, Integer startBucketNumber, + Integer startKeyNumber, int numberOfEntries) throws IOException { + TreeMap values = new TreeMap<>(); + List> keyValues = new ArrayList<>(); + String startKey = startVolumeNumber == null || startBucketNumber == null || startKeyNumber == null ? null + : (String.format("/%s%010d/%s%010d/%s%010d", volumeNamePrefix, startVolumeNumber, bucketNamePrefix, + startBucketNumber, keyPrefix, startKeyNumber)); + for (int i = 0; i < numberOfVolumes; i++) { + for (int j = 0; j < numberOfBucketsPerVolume; j++) { + for (int k = 0; k < numberOfKeysPerBucket; k++) { + String key = String.format("/%s%010d/%s%010d/%s%010d", volumeNamePrefix, i, bucketNamePrefix, j, + keyPrefix, k); + V value = valueClass == String.class ? (V) key : Mockito.mock(valueClass); + values.put(key, value); + + if ((volumeNumberFilter == null || i == volumeNumberFilter) && + (bucketNumberFilter == null || j == bucketNumberFilter) && + (startKey == null || startKey.compareTo(key) <= 0)) { + keyValues.add(Table.newKeyValue(key, value)); + } + } + } + } + + when(table.iterator(anyString())).thenAnswer(i -> new MapBackedTableIterator<>(values, i.getArgument(0))); + return keyValues.subList(0, Math.min(numberOfEntries, keyValues.size())); + } + + @ParameterizedTest + @MethodSource("getTableIteratorParameters") + @SuppressWarnings({"checkstyle:ParameterNumber"}) + public void testGetDeletedKeyEntries(int numberOfVolumes, int numberOfBucketsPerVolume, int numberOfKeysPerBucket, + Integer volumeNumber, Integer bucketNumber, + Integer startVolumeNumber, Integer startBucketNumber, Integer startKeyNumber, + int numberOfEntries, Class expectedException) + throws IOException { + String volumeNamePrefix = "volume"; + String bucketNamePrefix = "bucket"; + String keyPrefix = "key"; + OzoneConfiguration configuration = new OzoneConfiguration(); + OMMetadataManager metadataManager = Mockito.mock(OMMetadataManager.class); + when(metadataManager.getBucketKeyPrefix(anyString(), anyString())).thenAnswer(i -> + "/" + i.getArguments()[0] + "/" + i.getArguments()[1] + "/"); + KeyManagerImpl km = new KeyManagerImpl(null, null, metadataManager, configuration, null, null, null); + Table mockedDeletedTable = Mockito.mock(Table.class); + when(metadataManager.getDeletedTable()).thenReturn(mockedDeletedTable); + List>> expectedEntries = mockTableIterator( + RepeatedOmKeyInfo.class, mockedDeletedTable, numberOfVolumes, numberOfBucketsPerVolume, numberOfKeysPerBucket, + volumeNamePrefix, bucketNamePrefix, keyPrefix, volumeNumber, bucketNumber, startVolumeNumber, startBucketNumber, + startKeyNumber, numberOfEntries).stream() + .map(kv -> { + try { + String key = kv.getKey(); + RepeatedOmKeyInfo value = kv.getValue(); + List omKeyInfos = Collections.singletonList(Mockito.mock(OmKeyInfo.class)); + when(value.cloneOmKeyInfoList()).thenReturn(omKeyInfos); + return Table.newKeyValue(key, omKeyInfos); + } catch (IOException e) { + throw new RuntimeException(e); + } + }).collect(Collectors.toList()); + String volumeName = volumeNumber == null ? null : (String.format("%s%010d", volumeNamePrefix, volumeNumber)); + String bucketName = bucketNumber == null ? null : (String.format("%s%010d", bucketNamePrefix, bucketNumber)); + String startKey = startVolumeNumber == null || startBucketNumber == null || startKeyNumber == null ? null + : (String.format("/%s%010d/%s%010d/%s%010d", volumeNamePrefix, startVolumeNumber, bucketNamePrefix, + startBucketNumber, keyPrefix, startKeyNumber)); + if (expectedException != null) { + assertThrows(expectedException, () -> km.getDeletedKeyEntries(volumeName, bucketName, startKey, numberOfEntries)); + } else { + assertEquals(expectedEntries, km.getDeletedKeyEntries(volumeName, bucketName, startKey, numberOfEntries)); + } + } + + @ParameterizedTest + @MethodSource("getTableIteratorParameters") + @SuppressWarnings({"checkstyle:ParameterNumber"}) + public void testGetRenameKeyEntries(int numberOfVolumes, int numberOfBucketsPerVolume, int numberOfKeysPerBucket, + Integer volumeNumber, Integer bucketNumber, + Integer startVolumeNumber, Integer startBucketNumber, Integer startKeyNumber, + int numberOfEntries, Class expectedException) + throws IOException { + String volumeNamePrefix = "volume"; + String bucketNamePrefix = "bucket"; + String keyPrefix = ""; + OzoneConfiguration configuration = new OzoneConfiguration(); + OMMetadataManager metadataManager = Mockito.mock(OMMetadataManager.class); + when(metadataManager.getBucketKeyPrefix(anyString(), anyString())).thenAnswer(i -> + "/" + i.getArguments()[0] + "/" + i.getArguments()[1] + "/"); + KeyManagerImpl km = new KeyManagerImpl(null, null, metadataManager, configuration, null, null, null); + Table mockedRenameTable = Mockito.mock(Table.class); + when(metadataManager.getSnapshotRenamedTable()).thenReturn(mockedRenameTable); + List> expectedEntries = mockTableIterator( + String.class, mockedRenameTable, numberOfVolumes, numberOfBucketsPerVolume, numberOfKeysPerBucket, + volumeNamePrefix, bucketNamePrefix, keyPrefix, volumeNumber, bucketNumber, startVolumeNumber, startBucketNumber, + startKeyNumber, numberOfEntries); + String volumeName = volumeNumber == null ? null : (String.format("%s%010d", volumeNamePrefix, volumeNumber)); + String bucketName = bucketNumber == null ? null : (String.format("%s%010d", bucketNamePrefix, bucketNumber)); + String startKey = startVolumeNumber == null || startBucketNumber == null || startKeyNumber == null ? null + : (String.format("/%s%010d/%s%010d/%s%010d", volumeNamePrefix, startVolumeNumber, bucketNamePrefix, + startBucketNumber, keyPrefix, startKeyNumber)); + if (expectedException != null) { + assertThrows(expectedException, () -> km.getRenamesKeyEntries(volumeName, bucketName, startKey, numberOfEntries)); + } else { + assertEquals(expectedEntries, km.getRenamesKeyEntries(volumeName, bucketName, startKey, numberOfEntries)); + } + } + + @ParameterizedTest + @MethodSource("getTableIteratorParameters") + @SuppressWarnings({"checkstyle:ParameterNumber"}) + public void testGetDeletedDirEntries(int numberOfVolumes, int numberOfBucketsPerVolume, int numberOfKeysPerBucket, + Integer volumeNumber, Integer bucketNumber, + Integer startVolumeNumber, Integer startBucketNumber, Integer startKeyNumber, + int numberOfEntries, Class expectedException) + throws IOException { + String volumeNamePrefix = ""; + String bucketNamePrefix = ""; + String keyPrefix = "key"; + startVolumeNumber = null; + OzoneConfiguration configuration = new OzoneConfiguration(); + OMMetadataManager metadataManager = Mockito.mock(OMMetadataManager.class); + when(metadataManager.getBucketKeyPrefixFSO(anyString(), anyString())).thenAnswer(i -> + "/" + i.getArguments()[0] + "/" + i.getArguments()[1] + "/"); + KeyManagerImpl km = new KeyManagerImpl(null, null, metadataManager, configuration, null, null, null); + Table mockedDeletedDirTable = Mockito.mock(Table.class); + when(metadataManager.getDeletedDirTable()).thenReturn(mockedDeletedDirTable); + List> expectedEntries = mockTableIterator( + OmKeyInfo.class, mockedDeletedDirTable, numberOfVolumes, numberOfBucketsPerVolume, numberOfKeysPerBucket, + volumeNamePrefix, bucketNamePrefix, keyPrefix, volumeNumber, bucketNumber, startVolumeNumber, startBucketNumber, + startKeyNumber, numberOfEntries); + String volumeName = volumeNumber == null ? null : (String.format("%s%010d", volumeNamePrefix, volumeNumber)); + String bucketName = bucketNumber == null ? null : (String.format("%s%010d", bucketNamePrefix, bucketNumber)); + if (expectedException != null) { + assertThrows(expectedException, () -> km.getDeletedDirEntries(volumeName, bucketName, numberOfEntries)); + } else { + assertEquals(expectedEntries, km.getDeletedDirEntries(volumeName, bucketName, numberOfEntries)); + } + } +} From 192a0d1e3ae9356a809299ef5c70671ea59e510b Mon Sep 17 00:00:00 2001 From: "Doroszlai, Attila" Date: Thu, 3 Apr 2025 13:19:28 +0200 Subject: [PATCH 3/3] version not needed --- hadoop-ozone/ozone-manager/pom.xml | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-ozone/ozone-manager/pom.xml b/hadoop-ozone/ozone-manager/pom.xml index 5b6844f4d575..cc2aea097b1a 100644 --- a/hadoop-ozone/ozone-manager/pom.xml +++ b/hadoop-ozone/ozone-manager/pom.xml @@ -365,7 +365,6 @@ org.apache.ozone hdds-server-framework - ${hdds.version} test-jar test