From 11a011753fdea18d5f93f2abcf3f32ced1fd8a3a Mon Sep 17 00:00:00 2001 From: Istvan Fajth Date: Tue, 17 Nov 2020 17:32:23 +0100 Subject: [PATCH 01/10] HDDS-4478. Large deletedKeyset slows down OM via listStatus. --- .../response/file/OMFileCreateResponse.java | 6 + .../om/response/TestCleanupTableInfo.java | 222 ++++++++++++++++++ 2 files changed, 228 insertions(+) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMFileCreateResponse.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMFileCreateResponse.java index 88cbf8cb65d..bae6971eb80 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMFileCreateResponse.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMFileCreateResponse.java @@ -20,18 +20,24 @@ import javax.annotation.Nonnull; +import org.apache.hadoop.ozone.om.OmMetadataManagerImpl; import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; import org.apache.hadoop.ozone.om.helpers.OmKeyInfo; import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs; +import org.apache.hadoop.ozone.om.response.CleanupTableInfo; import org.apache.hadoop.ozone.om.response.key.OMKeyCreateResponse; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos .OMResponse; import java.util.List; +import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.KEY_TABLE; +import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.OPEN_KEY_TABLE; + /** * Response for crate file request. */ +@CleanupTableInfo(cleanupTables = {KEY_TABLE, OPEN_KEY_TABLE}) public class OMFileCreateResponse extends OMKeyCreateResponse { public OMFileCreateResponse(@Nonnull OMResponse omResponse, diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java index f66e3a3e1b7..b74a1ea4e3e 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java @@ -17,10 +17,33 @@ package org.apache.hadoop.ozone.om.response; +import com.google.common.collect.Iterators; +import org.apache.commons.lang3.tuple.Pair; +import org.apache.hadoop.hdds.client.BlockID; 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.protocol.proto.HddsProtos.DatanodeDetailsProto; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos.Pipeline; import org.apache.hadoop.hdds.server.ServerUtils; +import org.apache.hadoop.hdds.utils.db.cache.CacheKey; +import org.apache.hadoop.hdds.utils.db.cache.CacheValue; +import org.apache.hadoop.ozone.audit.AuditLogger; import org.apache.hadoop.ozone.om.OMMetadataManager; +import org.apache.hadoop.ozone.om.OMMetrics; import org.apache.hadoop.ozone.om.OmMetadataManagerImpl; +import org.apache.hadoop.ozone.om.OzoneManager; +import org.apache.hadoop.ozone.om.ResolvedBucket; +import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; +import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs; +import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper; +import org.apache.hadoop.ozone.om.request.file.OMFileCreateRequest; +import org.apache.hadoop.ozone.om.response.file.OMFileCreateResponse; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.CreateFileRequest; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.KeyArgs; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.KeyLocation; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type; import org.junit.Assert; import org.junit.Rule; import org.junit.Test; @@ -28,7 +51,23 @@ import org.reflections.Reflections; import java.io.File; +import java.io.IOException; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; import java.util.Set; +import java.util.UUID; + +import static org.junit.Assert.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.refEq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; /** * This tests check whether {@link OMClientResponse} have defined @@ -66,4 +105,187 @@ public void checkAnnotationAndTableName() throws Exception { } }); } + + @Test + public void testHDDS_4478() + throws Exception { + HddsProtos.BlockID blockID = new BlockID(1, 1).getProtobuf(); + String volume = "testVol"; + String bucket = "testBuck"; + String key = "/foo/bar/baz/key"; + + + OMFileCreateRequest request = + anOmFileCreateRequest(blockID, volume, bucket, key); + + OMMetadataManager omMetaMgr = createOMMetadataManagerSpy(); + OzoneManager om = + createOzoneManagerMock(volume, bucket, request, omMetaMgr); + + OmVolumeArgs volumeArgs = aVolumeArgs(volume); + OmBucketInfo bucketInfo = aBucketInfo(volume, bucket); + addVolumeToMetaTable(volume, volumeArgs, omMetaMgr); + addBucketToMetaTable(volume, bucket, bucketInfo, omMetaMgr); + + OzoneManagerDoubleBufferHelper dbh = + mock(OzoneManagerDoubleBufferHelper.class); + + Map cacheItemCount = new HashMap<>(); + for (String tableName : omMetaMgr.listTableNames()){ + cacheItemCount.put(tableName, + Iterators.size(omMetaMgr.getTable(tableName).cacheIterator())); + } + + + request.validateAndUpdateCache(om, 1, dbh); + + + CleanupTableInfo ann = + OMFileCreateResponse.class.getAnnotation(CleanupTableInfo.class); + List cleanup = Arrays.asList(ann.cleanupTables()); + for (String tableName : omMetaMgr.listTableNames()) { + if (!cleanup.contains(tableName)) { + assertEquals("Cache item count of table " +tableName, + cacheItemCount.get(tableName).intValue(), + Iterators.size(omMetaMgr.getTable(tableName).cacheIterator()) + ); + } + } + + verify(mock(OMMetrics.class), times(1)).incNumCreateFile(); + } + + private void addBucketToMetaTable(String volume, String bucket, + OmBucketInfo bucketInfo, OMMetadataManager omMetaMgr) throws IOException { + CacheValue cachedBucket = mock(CacheValue.class); + when(cachedBucket.getCacheValue()).thenReturn(bucketInfo); + String bucketKey = omMetaMgr.getBucketKey(volume, bucket); + omMetaMgr.getBucketTable().put(bucketKey, bucketInfo); + omMetaMgr.getBucketTable() + .addCacheEntry(new CacheKey<>(bucketKey), cachedBucket); + } + + private void addVolumeToMetaTable(String volume, OmVolumeArgs volumeArgs, + OMMetadataManager omMetaMgr) throws IOException { + CacheValue cachedVol = mock(CacheValue.class); + when(cachedVol.getCacheValue()).thenReturn(volumeArgs); + String volumeKey = omMetaMgr.getVolumeKey(volume); + omMetaMgr.getVolumeTable().put(volumeKey, volumeArgs); + omMetaMgr.getVolumeTable() + .addCacheEntry(new CacheKey<>(volumeKey), cachedVol); + } + + private OzoneManager createOzoneManagerMock(String volume, String bucket, + OMFileCreateRequest request, OMMetadataManager omMetaMgr) + throws IOException { + OzoneManager om = mock(OzoneManager.class); + when(om.getMetrics()).thenReturn(mock(OMMetrics.class)); + when(om.getMetadataManager()).thenReturn(omMetaMgr); + when(om.resolveBucketLink(any(KeyArgs.class), refEq(request))).thenAnswer( + invocationOnMock -> { + Pair pair = Pair.of(volume, bucket); + return new ResolvedBucket(pair, pair); + } + ); + when(om.getAclsEnabled()).thenReturn(false); + when(om.getAuditLogger()).thenReturn(mock(AuditLogger.class)); + return om; + } + + private OMMetadataManager createOMMetadataManagerSpy() throws IOException { + OzoneConfiguration conf = new OzoneConfiguration(); + File newFolder = folder.newFolder(); + if (!newFolder.exists()) { + Assert.assertTrue(newFolder.mkdirs()); + } + ServerUtils.setOzoneMetaDirPath(conf, newFolder.toString()); + OMMetadataManager omMetaMgr = spy(new OmMetadataManagerImpl(conf)); + return omMetaMgr; + } + + private OMFileCreateRequest anOmFileCreateRequest(HddsProtos.BlockID blockID, + String volume, String bucket, String key) { + OMRequest protoRequest = mock(OMRequest.class); + when(protoRequest.getCreateFileRequest()).thenReturn( + aCreateFileRequest(blockID, volume, bucket, key)); + when(protoRequest.getCmdType()).thenReturn(Type.CreateFile); + when(protoRequest.getTraceID()).thenReturn(""); + return new OMFileCreateRequest(protoRequest); + } + + private OmBucketInfo aBucketInfo(String volume, String bucket) { + return OmBucketInfo.newBuilder() + .setVolumeName(volume) + .setBucketName(bucket) + .setAcls(Collections.emptyList()) + .setIsVersionEnabled(false) + .setStorageType(StorageType.DEFAULT) + .build(); + } + + private OmVolumeArgs aVolumeArgs(String volume) { + return OmVolumeArgs.newBuilder() + .setAdminName("admin") + .setOwnerName("owner") + .setVolume(volume) + .build(); + } + + private CreateFileRequest aCreateFileRequest(HddsProtos.BlockID blockID, + String volume, String bucket, String key) { + return CreateFileRequest.newBuilder() + .setKeyArgs(aKeyArgs(blockID, volume, bucket, key)) + .setIsRecursive(true) + .setIsOverwrite(false) + .setClientID(1L) + .build(); + } + + private KeyArgs aKeyArgs(HddsProtos.BlockID blockID, String volume, + String bucket, String key) { + return KeyArgs.newBuilder() + .setVolumeName(volume) + .setBucketName(bucket) + .setKeyName(key) + .setDataSize(512L) + .addKeyLocations(aKeyLocation(blockID)) + .addKeyLocations(aKeyLocation(blockID)) + .addKeyLocations(aKeyLocation(blockID)) + .build(); + } + + private KeyLocation aKeyLocation( + HddsProtos.BlockID blockID) { + return KeyLocation.newBuilder() + .setBlockID(blockID) + .setOffset(0) + .setLength(512) + .setCreateVersion(0) + .setPipeline(aPipeline()) + .build(); + } + + private Pipeline aPipeline() { + return Pipeline.newBuilder() + .setId(aPipelineID()) + .addMembers(aDatanodeDetailsProto("192.168.1.1", "host1")) + .addMembers(aDatanodeDetailsProto("192.168.1.2", "host2")) + .addMembers(aDatanodeDetailsProto("192.168.1.3", "host3")) + .build(); + } + + private DatanodeDetailsProto aDatanodeDetailsProto(String s, + String host1) { + return DatanodeDetailsProto.newBuilder() + .setUuid(UUID.randomUUID().toString()) + .setIpAddress(s) + .setHostName(host1) + .build(); + } + + private HddsProtos.PipelineID aPipelineID() { + return HddsProtos.PipelineID.newBuilder() + .setId(UUID.randomUUID().toString()) + .build(); + } } From e1b8ab8c70a97ef08b817e01365bcc373b88b01f Mon Sep 17 00:00:00 2001 From: Istvan Fajth Date: Tue, 17 Nov 2020 18:01:42 +0100 Subject: [PATCH 02/10] Address checkstyle issues. --- .../hadoop/ozone/om/response/file/OMFileCreateResponse.java | 1 - .../apache/hadoop/ozone/om/response/TestCleanupTableInfo.java | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMFileCreateResponse.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMFileCreateResponse.java index bae6971eb80..de490c5aa85 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMFileCreateResponse.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/file/OMFileCreateResponse.java @@ -20,7 +20,6 @@ import javax.annotation.Nonnull; -import org.apache.hadoop.ozone.om.OmMetadataManagerImpl; import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; import org.apache.hadoop.ozone.om.helpers.OmKeyInfo; import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs; diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java index b74a1ea4e3e..e7a6d7cd080 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java @@ -107,8 +107,7 @@ public void checkAnnotationAndTableName() throws Exception { } @Test - public void testHDDS_4478() - throws Exception { + public void testHDDS4478() throws Exception { HddsProtos.BlockID blockID = new BlockID(1, 1).getProtobuf(); String volume = "testVol"; String bucket = "testBuck"; From 017180b96cdee7f6261246802817ce07d0394046 Mon Sep 17 00:00:00 2001 From: Istvan Fajth Date: Tue, 17 Nov 2020 19:17:50 +0100 Subject: [PATCH 03/10] Fix overdone mindless refactoring. :) --- .../ozone/om/response/TestCleanupTableInfo.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java index e7a6d7cd080..0a7cc3b60e5 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java @@ -118,8 +118,9 @@ public void testHDDS4478() throws Exception { anOmFileCreateRequest(blockID, volume, bucket, key); OMMetadataManager omMetaMgr = createOMMetadataManagerSpy(); + OMMetrics omMetrics = mock(OMMetrics.class); OzoneManager om = - createOzoneManagerMock(volume, bucket, request, omMetaMgr); + createOzoneManagerMock(volume, bucket, request, omMetaMgr, omMetrics); OmVolumeArgs volumeArgs = aVolumeArgs(volume); OmBucketInfo bucketInfo = aBucketInfo(volume, bucket); @@ -151,7 +152,7 @@ public void testHDDS4478() throws Exception { } } - verify(mock(OMMetrics.class), times(1)).incNumCreateFile(); + verify(omMetrics, times(1)).incNumCreateFile(); } private void addBucketToMetaTable(String volume, String bucket, @@ -175,10 +176,11 @@ private void addVolumeToMetaTable(String volume, OmVolumeArgs volumeArgs, } private OzoneManager createOzoneManagerMock(String volume, String bucket, - OMFileCreateRequest request, OMMetadataManager omMetaMgr) - throws IOException { + OMFileCreateRequest request, OMMetadataManager omMetaMgr, + OMMetrics metrics + ) throws IOException { OzoneManager om = mock(OzoneManager.class); - when(om.getMetrics()).thenReturn(mock(OMMetrics.class)); + when(om.getMetrics()).thenReturn(metrics); when(om.getMetadataManager()).thenReturn(omMetaMgr); when(om.resolveBucketLink(any(KeyArgs.class), refEq(request))).thenAnswer( invocationOnMock -> { From f87586ba60aaea42a06fdbea13c26706afce3ea6 Mon Sep 17 00:00:00 2001 From: Istvan Fajth Date: Tue, 17 Nov 2020 20:31:55 +0100 Subject: [PATCH 04/10] Trigger re-check. From c37e863cb207232de159ead29e7546702471da8c Mon Sep 17 00:00:00 2001 From: Istvan Fajth Date: Tue, 17 Nov 2020 21:55:16 +0100 Subject: [PATCH 05/10] Trigger re-check. From 763b1620dca909d297c2566ce55b70ae2971dddc Mon Sep 17 00:00:00 2001 From: Istvan Fajth Date: Tue, 17 Nov 2020 23:42:49 +0100 Subject: [PATCH 06/10] Trigger re-check. From d3f2f3e78b5c6387309a35d36dbf5175c192772d Mon Sep 17 00:00:00 2001 From: Istvan Fajth Date: Thu, 19 Nov 2020 03:38:45 +0100 Subject: [PATCH 07/10] Added Junit test for KeyCreateResponse case came from the review. Refactored the test to make the real test methods easy to understand and free from setup clutter as much as possible. --- .../om/response/key/OMKeyCreateResponse.java | 3 +- .../om/response/TestCleanupTableInfo.java | 273 +++++++++++------- 2 files changed, 177 insertions(+), 99 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCreateResponse.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCreateResponse.java index 2ae53591849..86224a1a0b6 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCreateResponse.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMKeyCreateResponse.java @@ -34,12 +34,13 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.KEY_TABLE; import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.OPEN_KEY_TABLE; /** * Response for CreateKey request. */ -@CleanupTableInfo(cleanupTables = OPEN_KEY_TABLE) +@CleanupTableInfo(cleanupTables = {OPEN_KEY_TABLE, KEY_TABLE}) public class OMKeyCreateResponse extends OMClientResponse { public static final Logger LOG = diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java index 0a7cc3b60e5..980501e87f0 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java @@ -17,6 +17,7 @@ package org.apache.hadoop.ozone.om.response; +import com.google.common.base.Optional; import com.google.common.collect.Iterators; import org.apache.commons.lang3.tuple.Pair; import org.apache.hadoop.hdds.client.BlockID; @@ -37,17 +38,25 @@ import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; import org.apache.hadoop.ozone.om.helpers.OmVolumeArgs; import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper; +import org.apache.hadoop.ozone.om.request.OMClientRequest; import org.apache.hadoop.ozone.om.request.file.OMFileCreateRequest; +import org.apache.hadoop.ozone.om.request.key.OMKeyCreateRequest; import org.apache.hadoop.ozone.om.response.file.OMFileCreateResponse; +import org.apache.hadoop.ozone.om.response.key.OMKeyCreateResponse; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.CreateFileRequest; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.CreateKeyRequest; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.KeyArgs; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.KeyLocation; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type; import org.junit.Assert; +import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; import org.reflections.Reflections; import java.io.File; @@ -62,7 +71,6 @@ import static org.junit.Assert.assertEquals; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.refEq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; @@ -73,26 +81,66 @@ * This tests check whether {@link OMClientResponse} have defined * {@link CleanupTableInfo} annotation. */ +@RunWith(MockitoJUnitRunner.class) public class TestCleanupTableInfo { + private static final String TEST_VOLUME_NAME = "testVol"; + private static final String TEST_BUCKET_NAME = "testBucket"; + private static final String TEST_KEY = "/foo/bar/baz/key"; + private static final HddsProtos.BlockID TEST_BLOCK_ID = + new BlockID(1, 1).getProtobuf(); + public static final String OM_RESPONSE_PACKAGE = + "org.apache.hadoop.ozone.om.response"; @Rule public TemporaryFolder folder = new TemporaryFolder(); + @Mock + public OMMetrics omMetrics; + + @Mock + public OzoneManagerDoubleBufferHelper dbh; + + public OzoneManager om; + + /** + * Creates a mock Ozone Manager object. + * Defined behaviour in the mock: + * - returns the specified metrics instance + * - returns the specified metadataManager + * - resolves the bucket links to themselves (no symlinks) + * - disables ACLs + * - provides an audit logger + * + * @return the mocked Ozone Manager + * @throws IOException should not happen but declared in mocked methods + */ + @Before + public void setupOzoneManagerMock() + throws IOException { + om = mock(OzoneManager.class); + OMMetadataManager metaMgr = createOMMetadataManagerSpy(); + when(om.getMetrics()).thenReturn(omMetrics); + when(om.getMetadataManager()).thenReturn(metaMgr); + when(om.resolveBucketLink(any(KeyArgs.class), any(OMClientRequest.class))) + .thenAnswer( + invocationOnMock -> { + Pair pair = + Pair.of(TEST_VOLUME_NAME, TEST_BUCKET_NAME); + return new ResolvedBucket(pair, pair); + } + ); + when(om.getAclsEnabled()).thenReturn(false); + when(om.getAuditLogger()).thenReturn(mock(AuditLogger.class)); + addVolumeToMetaTable(aVolumeArgs()); + addBucketToMetaTable(aBucketInfo()); + } + @Test - public void checkAnnotationAndTableName() throws Exception { - OzoneConfiguration conf = new OzoneConfiguration(); - File newFolder = folder.newFolder(); - if (!newFolder.exists()) { - Assert.assertTrue(newFolder.mkdirs()); - } - ServerUtils.setOzoneMetaDirPath(conf, newFolder.toString()); - OMMetadataManager omMetadataManager = new OmMetadataManagerImpl(conf); + public void checkAnnotationAndTableName() { + OMMetadataManager omMetadataManager = om.getMetadataManager(); Set tables = omMetadataManager.listTableNames(); - Reflections reflections = new Reflections( - "org.apache.hadoop.ozone.om.response"); - Set> subTypes = - reflections.getSubTypesOf(OMClientResponse.class); + Set> subTypes = responseClasses(); subTypes.forEach(aClass -> { Assert.assertTrue(aClass + "does not have annotation of" + " CleanupTableInfo", @@ -106,93 +154,112 @@ public void checkAnnotationAndTableName() throws Exception { }); } + private Set> responseClasses() { + Reflections reflections = new Reflections(OM_RESPONSE_PACKAGE); + return reflections.getSubTypesOf(OMClientResponse.class); + } + @Test - public void testHDDS4478() throws Exception { - HddsProtos.BlockID blockID = new BlockID(1, 1).getProtobuf(); - String volume = "testVol"; - String bucket = "testBuck"; - String key = "/foo/bar/baz/key"; + public void testFileCreateRequestSetsAllTouchedTableCachesForEviction() { + OMFileCreateRequest request = anOMFileCreateRequest(); + Map cacheItemCount = recordCacheItemCounts(); + request.validateAndUpdateCache(om, 1, dbh); - OMFileCreateRequest request = - anOmFileCreateRequest(blockID, volume, bucket, key); + assertCacheItemCounts(cacheItemCount, OMFileCreateResponse.class); + verify(omMetrics, times(1)).incNumCreateFile(); + } - OMMetadataManager omMetaMgr = createOMMetadataManagerSpy(); - OMMetrics omMetrics = mock(OMMetrics.class); - OzoneManager om = - createOzoneManagerMock(volume, bucket, request, omMetaMgr, omMetrics); + @Test + public void testKeyCreateRequestSetsAllTouchedTableCachesForEviction() { + OMKeyCreateRequest request = anOMKeyCreateRequest(); + when(om.getEnableFileSystemPaths()).thenReturn(true); - OmVolumeArgs volumeArgs = aVolumeArgs(volume); - OmBucketInfo bucketInfo = aBucketInfo(volume, bucket); - addVolumeToMetaTable(volume, volumeArgs, omMetaMgr); - addBucketToMetaTable(volume, bucket, bucketInfo, omMetaMgr); + Map cacheItemCount = recordCacheItemCounts(); - OzoneManagerDoubleBufferHelper dbh = - mock(OzoneManagerDoubleBufferHelper.class); + request.validateAndUpdateCache(om, 1, dbh); - Map cacheItemCount = new HashMap<>(); - for (String tableName : omMetaMgr.listTableNames()){ - cacheItemCount.put(tableName, - Iterators.size(omMetaMgr.getTable(tableName).cacheIterator())); - } + assertCacheItemCounts(cacheItemCount, OMKeyCreateResponse.class); + verify(omMetrics, times(1)).incNumKeyAllocates(); + } - request.validateAndUpdateCache(om, 1, dbh); + private Map recordCacheItemCounts() { + Map cacheItemCount = new HashMap<>(); + for (String tableName : om.getMetadataManager().listTableNames()) { + cacheItemCount.put( + tableName, + Iterators.size( + om.getMetadataManager().getTable(tableName).cacheIterator() + ) + ); + } + return cacheItemCount; + } - CleanupTableInfo ann = - OMFileCreateResponse.class.getAnnotation(CleanupTableInfo.class); + private void assertCacheItemCounts(Map cacheItemCount, + Class responseClass) + { + CleanupTableInfo ann = responseClass.getAnnotation(CleanupTableInfo.class); List cleanup = Arrays.asList(ann.cleanupTables()); - for (String tableName : omMetaMgr.listTableNames()) { + for (String tableName : om.getMetadataManager().listTableNames()) { if (!cleanup.contains(tableName)) { - assertEquals("Cache item count of table " +tableName, + assertEquals( + "Cache item count of table " +tableName, cacheItemCount.get(tableName).intValue(), - Iterators.size(omMetaMgr.getTable(tableName).cacheIterator()) + Iterators.size( + om.getMetadataManager().getTable(tableName).cacheIterator() + ) ); } } - - verify(omMetrics, times(1)).incNumCreateFile(); } - private void addBucketToMetaTable(String volume, String bucket, - OmBucketInfo bucketInfo, OMMetadataManager omMetaMgr) throws IOException { - CacheValue cachedBucket = mock(CacheValue.class); - when(cachedBucket.getCacheValue()).thenReturn(bucketInfo); - String bucketKey = omMetaMgr.getBucketKey(volume, bucket); - omMetaMgr.getBucketTable().put(bucketKey, bucketInfo); - omMetaMgr.getBucketTable() - .addCacheEntry(new CacheKey<>(bucketKey), cachedBucket); - } - - private void addVolumeToMetaTable(String volume, OmVolumeArgs volumeArgs, - OMMetadataManager omMetaMgr) throws IOException { - CacheValue cachedVol = mock(CacheValue.class); - when(cachedVol.getCacheValue()).thenReturn(volumeArgs); - String volumeKey = omMetaMgr.getVolumeKey(volume); - omMetaMgr.getVolumeTable().put(volumeKey, volumeArgs); - omMetaMgr.getVolumeTable() - .addCacheEntry(new CacheKey<>(volumeKey), cachedVol); + /** + * Adds the volume info to the volumeTable in the MetadataManager, and also + * add the value to the table's cache. + * + * @param volumeArgs the OMVolumeArgs object specifying the volume propertes + * @throws IOException if an IO issue occurs while wrtiing to RocksDB + */ + private void addVolumeToMetaTable(OmVolumeArgs volumeArgs) + throws IOException { + String volumeKey = om.getMetadataManager().getVolumeKey(TEST_VOLUME_NAME); + om.getMetadataManager().getVolumeTable().put(volumeKey, volumeArgs); + om.getMetadataManager().getVolumeTable().addCacheEntry( + new CacheKey<>(volumeKey), + new CacheValue<>(Optional.of(volumeArgs), 2) + ); } - private OzoneManager createOzoneManagerMock(String volume, String bucket, - OMFileCreateRequest request, OMMetadataManager omMetaMgr, - OMMetrics metrics - ) throws IOException { - OzoneManager om = mock(OzoneManager.class); - when(om.getMetrics()).thenReturn(metrics); - when(om.getMetadataManager()).thenReturn(omMetaMgr); - when(om.resolveBucketLink(any(KeyArgs.class), refEq(request))).thenAnswer( - invocationOnMock -> { - Pair pair = Pair.of(volume, bucket); - return new ResolvedBucket(pair, pair); - } + /** + * Adds the bucket info to the bucketTable in the MetadataManager, and also + * adds the value to the table's cache. + * + * @param bucketInfo the OMBucketInfo object specifying the bucket properties + * @throws IOException if an IO issue occurs while writing to RocksDB + */ + private void addBucketToMetaTable(OmBucketInfo bucketInfo) + throws IOException { + String bucketKey = om.getMetadataManager() + .getBucketKey(bucketInfo.getVolumeName(), bucketInfo.getBucketName()); + om.getMetadataManager().getBucketTable().put(bucketKey, bucketInfo); + om.getMetadataManager().getBucketTable().addCacheEntry( + new CacheKey<>(bucketKey), + new CacheValue<>(Optional.of(bucketInfo), 1) ); - when(om.getAclsEnabled()).thenReturn(false); - when(om.getAuditLogger()).thenReturn(mock(AuditLogger.class)); - return om; } + /** + * Creates a spy object over an instantiated OMMetadataManager, giving the + * possibility to redefine behaviour. In the current implementation + * there isn't any behaviour which is redefined. + * + * @return the OMMetadataManager spy instance created. + * @throws IOException if I/O error occurs in setting up data store for the + * metadata manager. + */ private OMMetadataManager createOMMetadataManagerSpy() throws IOException { OzoneConfiguration conf = new OzoneConfiguration(); File newFolder = folder.newFolder(); @@ -200,58 +267,68 @@ private OMMetadataManager createOMMetadataManagerSpy() throws IOException { Assert.assertTrue(newFolder.mkdirs()); } ServerUtils.setOzoneMetaDirPath(conf, newFolder.toString()); - OMMetadataManager omMetaMgr = spy(new OmMetadataManagerImpl(conf)); - return omMetaMgr; + return spy(new OmMetadataManagerImpl(conf)); } - private OMFileCreateRequest anOmFileCreateRequest(HddsProtos.BlockID blockID, - String volume, String bucket, String key) { + private OMFileCreateRequest anOMFileCreateRequest() { OMRequest protoRequest = mock(OMRequest.class); - when(protoRequest.getCreateFileRequest()).thenReturn( - aCreateFileRequest(blockID, volume, bucket, key)); + when(protoRequest.getCreateFileRequest()).thenReturn(aCreateFileRequest()); when(protoRequest.getCmdType()).thenReturn(Type.CreateFile); when(protoRequest.getTraceID()).thenReturn(""); return new OMFileCreateRequest(protoRequest); } - private OmBucketInfo aBucketInfo(String volume, String bucket) { + private OMKeyCreateRequest anOMKeyCreateRequest(){ + OMRequest protoRequest = mock(OMRequest.class); + when(protoRequest.getCreateKeyRequest()).thenReturn(aKeyCreateRequest()); + when(protoRequest.getCmdType()).thenReturn(Type.CreateKey); + when(protoRequest.getTraceID()).thenReturn(""); + return new OMKeyCreateRequest(protoRequest); + } + + private OmBucketInfo aBucketInfo() { return OmBucketInfo.newBuilder() - .setVolumeName(volume) - .setBucketName(bucket) + .setVolumeName(TEST_VOLUME_NAME) + .setBucketName(TEST_BUCKET_NAME) .setAcls(Collections.emptyList()) .setIsVersionEnabled(false) .setStorageType(StorageType.DEFAULT) .build(); } - private OmVolumeArgs aVolumeArgs(String volume) { + private OmVolumeArgs aVolumeArgs() { return OmVolumeArgs.newBuilder() .setAdminName("admin") .setOwnerName("owner") - .setVolume(volume) + .setVolume(TEST_VOLUME_NAME) .build(); } - private CreateFileRequest aCreateFileRequest(HddsProtos.BlockID blockID, - String volume, String bucket, String key) { + private CreateFileRequest aCreateFileRequest() { return CreateFileRequest.newBuilder() - .setKeyArgs(aKeyArgs(blockID, volume, bucket, key)) + .setKeyArgs(aKeyArgs()) .setIsRecursive(true) .setIsOverwrite(false) .setClientID(1L) .build(); } - private KeyArgs aKeyArgs(HddsProtos.BlockID blockID, String volume, - String bucket, String key) { + private CreateKeyRequest aKeyCreateRequest() { + return CreateKeyRequest.newBuilder() + .setKeyArgs(aKeyArgs()) + .setClientID(1L) + .build(); + } + + private KeyArgs aKeyArgs() { return KeyArgs.newBuilder() - .setVolumeName(volume) - .setBucketName(bucket) - .setKeyName(key) + .setVolumeName(TEST_VOLUME_NAME) + .setBucketName(TEST_BUCKET_NAME) + .setKeyName(TEST_KEY) .setDataSize(512L) - .addKeyLocations(aKeyLocation(blockID)) - .addKeyLocations(aKeyLocation(blockID)) - .addKeyLocations(aKeyLocation(blockID)) + .addKeyLocations(aKeyLocation(TEST_BLOCK_ID)) + .addKeyLocations(aKeyLocation(TEST_BLOCK_ID)) + .addKeyLocations(aKeyLocation(TEST_BLOCK_ID)) .build(); } From e403887f4db52ab65a77c0dbdb8125da035be468 Mon Sep 17 00:00:00 2001 From: Istvan Fajth Date: Thu, 19 Nov 2020 03:45:32 +0100 Subject: [PATCH 08/10] Fix checkstyle warnings. --- .../ozone/om/response/TestCleanupTableInfo.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java index 980501e87f0..a59a4e1ac72 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java @@ -78,8 +78,10 @@ import static org.mockito.Mockito.when; /** - * This tests check whether {@link OMClientResponse} have defined + * The test checks whether all {@link OMClientResponse} have defined the * {@link CleanupTableInfo} annotation. + * For certain requests it check whether it is properly defined not just the + * fact that it is defined. */ @RunWith(MockitoJUnitRunner.class) public class TestCleanupTableInfo { @@ -95,12 +97,12 @@ public class TestCleanupTableInfo { public TemporaryFolder folder = new TemporaryFolder(); @Mock - public OMMetrics omMetrics; + private OMMetrics omMetrics; @Mock - public OzoneManagerDoubleBufferHelper dbh; + private OzoneManagerDoubleBufferHelper dbh; - public OzoneManager om; + private OzoneManager om; /** * Creates a mock Ozone Manager object. @@ -199,8 +201,8 @@ private Map recordCacheItemCounts() { } private void assertCacheItemCounts(Map cacheItemCount, - Class responseClass) - { + Class responseClass + ) { CleanupTableInfo ann = responseClass.getAnnotation(CleanupTableInfo.class); List cleanup = Arrays.asList(ann.cleanupTables()); for (String tableName : om.getMetadataManager().listTableNames()) { From e2455eb167f455cc7bd35ea2a1500851c4d6002f Mon Sep 17 00:00:00 2001 From: Istvan Fajth Date: Thu, 19 Nov 2020 11:40:26 +0100 Subject: [PATCH 09/10] Trigger re-check. From ae46f08910cc5c9966f69e20a37e1ee35aad7e05 Mon Sep 17 00:00:00 2001 From: Istvan Fajth Date: Thu, 19 Nov 2020 12:48:21 +0100 Subject: [PATCH 10/10] Trigger re-check.