From 9c72bf462196e1d71a243903b74e3c4673f29efb Mon Sep 17 00:00:00 2001 From: Bharat Viswanadham Date: Thu, 10 Oct 2019 16:49:06 -0700 Subject: [PATCH] HDDS-1986. Fix listkeys API. (#1588) --- .../ozone/om/OmMetadataManagerImpl.java | 89 ++++++- .../ozone/om/TestOmMetadataManager.java | 226 ++++++++++++++++++ .../ozone/om/request/TestOMRequestUtils.java | 60 ++++- 3 files changed, 352 insertions(+), 23 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java index d784dbe218..95f21ae0ca 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java @@ -23,6 +23,9 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.TreeMap; +import java.util.TreeSet; import java.util.stream.Collectors; import org.apache.hadoop.hdds.client.BlockID; @@ -653,7 +656,12 @@ public List listBuckets(final String volumeName, @Override public List listKeys(String volumeName, String bucketName, String startKey, String keyPrefix, int maxKeys) throws IOException { + List result = new ArrayList<>(); + if (maxKeys <= 0) { + return result; + } + if (Strings.isNullOrEmpty(volumeName)) { throw new OMException("Volume name is required.", ResultCodes.VOLUME_NOT_FOUND); @@ -688,19 +696,56 @@ public List listKeys(String volumeName, String bucketName, seekPrefix = getBucketKey(volumeName, bucketName + OM_KEY_PREFIX); } int currentCount = 0; - try (TableIterator> keyIter = - getKeyTable() - .iterator()) { - KeyValue kv = keyIter.seek(seekKey); - while (currentCount < maxKeys && keyIter.hasNext()) { - kv = keyIter.next(); - // Skip the Start key if needed. - if (kv != null && skipStartKey && kv.getKey().equals(seekKey)) { - continue; + + + TreeMap cacheKeyMap = new TreeMap<>(); + Set deletedKeySet = new TreeSet<>(); + Iterator, CacheValue>> iterator = + keyTable.cacheIterator(); + + //TODO: We can avoid this iteration if table cache has stored entries in + // treemap. Currently HashMap is used in Cache. HashMap get operation is an + // constant time operation, where as for treeMap get is log(n). + // So if we move to treemap, the get operation will be affected. As get + // is frequent operation on table. So, for now in list we iterate cache map + // and construct treeMap which match with keyPrefix and are greater than or + // equal to startKey. Later we can revisit this, if list operation + // is becoming slow. + while (iterator.hasNext()) { + Map.Entry< CacheKey, CacheValue> entry = + iterator.next(); + + String key = entry.getKey().getCacheKey(); + OmKeyInfo omKeyInfo = entry.getValue().getCacheValue(); + // Making sure that entry in cache is not for delete key request. + + if (omKeyInfo != null) { + if (key.startsWith(seekPrefix) && key.compareTo(seekKey) >= 0) { + cacheKeyMap.put(key, omKeyInfo); } + } else { + deletedKeySet.add(key); + } + } + + // Get maxKeys from DB if it has. + + try (TableIterator> + keyIter = getKeyTable().iterator()) { + KeyValue< String, OmKeyInfo > kv; + keyIter.seek(seekKey); + // we need to iterate maxKeys + 1 here because if skipStartKey is true, + // we should skip that entry and return the result. + while (currentCount < maxKeys + 1 && keyIter.hasNext()) { + kv = keyIter.next(); if (kv != null && kv.getKey().startsWith(seekPrefix)) { - result.add(kv.getValue()); - currentCount++; + + // Entry should not be marked for delete, consider only those + // entries. + if(!deletedKeySet.contains(kv.getKey())) { + cacheKeyMap.put(kv.getKey(), kv.getValue()); + currentCount++; + } } else { // The SeekPrefix does not match any more, we can break out of the // loop. @@ -708,6 +753,28 @@ public List listKeys(String volumeName, String bucketName, } } } + + // Finally DB entries and cache entries are merged, then return the count + // of maxKeys from the sorted map. + currentCount = 0; + + for (Map.Entry cacheKey : cacheKeyMap.entrySet()) { + if (cacheKey.getKey().equals(seekKey) && skipStartKey) { + continue; + } + + result.add(cacheKey.getValue()); + currentCount++; + + if (currentCount == maxKeys) { + break; + } + } + + // Clear map and set. + cacheKeyMap.clear(); + deletedKeySet.clear(); + return result; } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmMetadataManager.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmMetadataManager.java index ed39410841..e0e4c61d3e 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmMetadataManager.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmMetadataManager.java @@ -19,9 +19,11 @@ import com.google.common.base.Optional; import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.protocol.StorageType; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos; import org.apache.hadoop.hdds.utils.db.cache.CacheKey; import org.apache.hadoop.hdds.utils.db.cache.CacheValue; import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; +import org.apache.hadoop.ozone.om.helpers.OmKeyInfo; import org.apache.hadoop.ozone.om.request.TestOMRequestUtils; import org.junit.Assert; import org.junit.Before; @@ -188,4 +190,228 @@ private void addBucketsToCache(String volumeName, String bucketName) { new CacheValue<>(Optional.of(omBucketInfo), 1)); } + @Test + public void testListKeys() throws Exception { + + String volumeNameA = "volumeA"; + String volumeNameB = "volumeB"; + String ozoneBucket = "ozoneBucket"; + String hadoopBucket = "hadoopBucket"; + + + // Create volumes and buckets. + TestOMRequestUtils.addVolumeToDB(volumeNameA, omMetadataManager); + TestOMRequestUtils.addVolumeToDB(volumeNameB, omMetadataManager); + addBucketsToCache(volumeNameA, ozoneBucket); + addBucketsToCache(volumeNameB, hadoopBucket); + + + String prefixKeyA = "key-a"; + String prefixKeyB = "key-b"; + TreeSet keysASet = new TreeSet<>(); + TreeSet keysBSet = new TreeSet<>(); + for (int i=1; i<= 100; i++) { + if (i % 2 == 0) { + keysASet.add( + prefixKeyA + i); + addKeysToOM(volumeNameA, ozoneBucket, prefixKeyA + i, i); + } else { + keysBSet.add( + prefixKeyB + i); + addKeysToOM(volumeNameA, hadoopBucket, prefixKeyB + i, i); + } + } + + + TreeSet keysAVolumeBSet = new TreeSet<>(); + TreeSet keysBVolumeBSet = new TreeSet<>(); + for (int i=1; i<= 100; i++) { + if (i % 2 == 0) { + keysAVolumeBSet.add( + prefixKeyA + i); + addKeysToOM(volumeNameB, ozoneBucket, prefixKeyA + i, i); + } else { + keysBVolumeBSet.add( + prefixKeyB + i); + addKeysToOM(volumeNameB, hadoopBucket, prefixKeyB + i, i); + } + } + + + // List all keys which have prefix "key-a" + List omKeyInfoList = + omMetadataManager.listKeys(volumeNameA, ozoneBucket, + null, prefixKeyA, 100); + + Assert.assertEquals(omKeyInfoList.size(), 50); + + for (OmKeyInfo omKeyInfo : omKeyInfoList) { + Assert.assertTrue(omKeyInfo.getKeyName().startsWith( + prefixKeyA)); + } + + + String startKey = prefixKeyA + 10; + omKeyInfoList = + omMetadataManager.listKeys(volumeNameA, ozoneBucket, + startKey, prefixKeyA, 100); + + Assert.assertEquals(keysASet.tailSet( + startKey).size() - 1, omKeyInfoList.size()); + + startKey = prefixKeyA + 38; + omKeyInfoList = + omMetadataManager.listKeys(volumeNameA, ozoneBucket, + startKey, prefixKeyA, 100); + + Assert.assertEquals(keysASet.tailSet( + startKey).size() - 1, omKeyInfoList.size()); + + for (OmKeyInfo omKeyInfo : omKeyInfoList) { + Assert.assertTrue(omKeyInfo.getKeyName().startsWith( + prefixKeyA)); + Assert.assertFalse(omKeyInfo.getBucketName().equals( + prefixKeyA + 38)); + } + + + + omKeyInfoList = omMetadataManager.listKeys(volumeNameB, hadoopBucket, + null, prefixKeyB, 100); + + Assert.assertEquals(omKeyInfoList.size(), 50); + + for (OmKeyInfo omKeyInfo : omKeyInfoList) { + Assert.assertTrue(omKeyInfo.getKeyName().startsWith( + prefixKeyB)); + } + + // Try to get keys by count 10, like that get all keys in the + // volumeB/ozoneBucket with "key-a". + startKey = null; + TreeSet expectedKeys = new TreeSet<>(); + for (int i=0; i<5; i++) { + + omKeyInfoList = omMetadataManager.listKeys(volumeNameB, hadoopBucket, + startKey, prefixKeyB, 10); + + Assert.assertEquals(10, omKeyInfoList.size()); + + for (OmKeyInfo omKeyInfo : omKeyInfoList) { + expectedKeys.add(omKeyInfo.getKeyName()); + Assert.assertTrue(omKeyInfo.getKeyName().startsWith( + prefixKeyB)); + startKey = omKeyInfo.getKeyName(); + } + } + + Assert.assertEquals(expectedKeys, keysBVolumeBSet); + + + // As now we have iterated all 50 buckets, calling next time should + // return empty list. + omKeyInfoList = omMetadataManager.listKeys(volumeNameB, hadoopBucket, + startKey, prefixKeyB, 10); + + Assert.assertEquals(omKeyInfoList.size(), 0); + + } + + @Test + public void testListKeysWithFewDeleteEntriesInCache() throws Exception { + String volumeNameA = "volumeA"; + String ozoneBucket = "ozoneBucket"; + + // Create volumes and bucket. + TestOMRequestUtils.addVolumeToDB(volumeNameA, omMetadataManager); + + addBucketsToCache(volumeNameA, ozoneBucket); + + String prefixKeyA = "key-a"; + TreeSet keysASet = new TreeSet<>(); + TreeSet deleteKeySet = new TreeSet<>(); + + + for (int i=1; i<= 100; i++) { + if (i % 2 == 0) { + keysASet.add( + prefixKeyA + i); + addKeysToOM(volumeNameA, ozoneBucket, prefixKeyA + i, i); + } else { + addKeysToOM(volumeNameA, ozoneBucket, prefixKeyA + i, i); + String key = omMetadataManager.getOzoneKey(volumeNameA, + ozoneBucket, prefixKeyA + i); + // Mark as deleted in cache. + omMetadataManager.getKeyTable().addCacheEntry( + new CacheKey<>(key), + new CacheValue<>(Optional.absent(), 100L)); + deleteKeySet.add(key); + } + } + + // Now list keys which match with prefixKeyA. + List omKeyInfoList = + omMetadataManager.listKeys(volumeNameA, ozoneBucket, + null, prefixKeyA, 100); + + // As in total 100, 50 are marked for delete. It should list only 50 keys. + Assert.assertEquals(50, omKeyInfoList.size()); + + TreeSet expectedKeys = new TreeSet<>(); + + for (OmKeyInfo omKeyInfo : omKeyInfoList) { + expectedKeys.add(omKeyInfo.getKeyName()); + Assert.assertTrue(omKeyInfo.getKeyName().startsWith(prefixKeyA)); + } + + Assert.assertEquals(expectedKeys, keysASet); + + + // Now get key count by 10. + String startKey = null; + expectedKeys = new TreeSet<>(); + for (int i=0; i<5; i++) { + + omKeyInfoList = omMetadataManager.listKeys(volumeNameA, ozoneBucket, + startKey, prefixKeyA, 10); + + System.out.println(i); + Assert.assertEquals(10, omKeyInfoList.size()); + + for (OmKeyInfo omKeyInfo : omKeyInfoList) { + expectedKeys.add(omKeyInfo.getKeyName()); + Assert.assertTrue(omKeyInfo.getKeyName().startsWith( + prefixKeyA)); + startKey = omKeyInfo.getKeyName(); + } + } + + Assert.assertEquals(keysASet, expectedKeys); + + + // As now we have iterated all 50 buckets, calling next time should + // return empty list. + omKeyInfoList = omMetadataManager.listKeys(volumeNameA, ozoneBucket, + startKey, prefixKeyA, 10); + + Assert.assertEquals(omKeyInfoList.size(), 0); + + + + } + + private void addKeysToOM(String volumeName, String bucketName, + String keyName, int i) throws Exception { + + if (i%2== 0) { + TestOMRequestUtils.addKeyToTable(false, volumeName, bucketName, keyName, + 1000L, HddsProtos.ReplicationType.RATIS, + HddsProtos.ReplicationFactor.ONE, omMetadataManager); + } else { + TestOMRequestUtils.addKeyToTableCache(volumeName, bucketName, keyName, + HddsProtos.ReplicationType.RATIS, HddsProtos.ReplicationFactor.ONE, + omMetadataManager); + } + } + } \ No newline at end of file diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/TestOMRequestUtils.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/TestOMRequestUtils.java index 88848f8b2a..472d46a289 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/TestOMRequestUtils.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/TestOMRequestUtils.java @@ -120,7 +120,52 @@ public static void addKeyToTable(boolean openKeyTable, String volumeName, OMMetadataManager omMetadataManager) throws Exception { - OmKeyInfo.Builder builder = new OmKeyInfo.Builder() + OmKeyInfo omKeyInfo = createOmKeyInfo(volumeName, bucketName, keyName, + replicationType, replicationFactor); + + if (openKeyTable) { + omMetadataManager.getOpenKeyTable().put( + omMetadataManager.getOpenKey(volumeName, bucketName, keyName, + clientID), omKeyInfo); + } else { + omMetadataManager.getKeyTable().put(omMetadataManager.getOzoneKey( + volumeName, bucketName, keyName), omKeyInfo); + } + + } + + /** + * Add key entry to key table cache. + * @param volumeName + * @param bucketName + * @param keyName + * @param replicationType + * @param replicationFactor + * @param omMetadataManager + */ + @SuppressWarnings("parameterNumber") + public static void addKeyToTableCache(String volumeName, + String bucketName, + String keyName, + HddsProtos.ReplicationType replicationType, + HddsProtos.ReplicationFactor replicationFactor, + OMMetadataManager omMetadataManager) { + + + OmKeyInfo omKeyInfo = createOmKeyInfo(volumeName, bucketName, keyName, + replicationType, replicationFactor); + + omMetadataManager.getKeyTable().addCacheEntry( + new CacheKey<>(omMetadataManager.getOzoneKey(volumeName, bucketName, + keyName)), new CacheValue<>(Optional.of(omKeyInfo), + 1L)); + + } + + private OmKeyInfo createKeyInfo(String volumeName, String bucketName, + String keyName, HddsProtos.ReplicationType replicationType, + HddsProtos.ReplicationFactor replicationFactor) { + return new OmKeyInfo.Builder() .setVolumeName(volumeName) .setBucketName(bucketName) .setKeyName(keyName) @@ -130,19 +175,10 @@ public static void addKeyToTable(boolean openKeyTable, String volumeName, .setModificationTime(Time.now()) .setDataSize(1000L) .setReplicationType(replicationType) - .setReplicationFactor(replicationFactor); - - if (openKeyTable) { - omMetadataManager.getOpenKeyTable().put( - omMetadataManager.getOpenKey(volumeName, bucketName, keyName, - clientID), builder.build()); - } else { - omMetadataManager.getKeyTable().put(omMetadataManager.getOzoneKey( - volumeName, bucketName, keyName), builder.build()); - } - + .setReplicationFactor(replicationFactor).build(); } + /** * Create OmKeyInfo. */