From 6b6424296bfae5e6186fffb6f93365e74ff17ba8 Mon Sep 17 00:00:00 2001 From: cxorm Date: Mon, 7 Oct 2019 23:25:40 +0800 Subject: [PATCH 1/5] HDDS-1737. Add Volume check in KeyManager and File Operations. --- .../file/OMDirectoryCreateRequest.java | 11 ++++++---- .../om/request/key/OMKeyDeleteRequest.java | 21 ++++++++++++++----- .../om/request/key/OMKeyRenameRequest.java | 18 ++++++++++++---- 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java index 4b591dbed2d5..3cb74f9a88c2 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java @@ -63,8 +63,9 @@ import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.BUCKET_NOT_FOUND; +import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.VOLUME_NOT_FOUND; import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.FILE_ALREADY_EXISTS; -import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK; +import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK; import static org.apache.hadoop.ozone.om.request.file.OMFileRequest.OMDirectoryResult.DIRECTORY_EXISTS_IN_GIVENPATH; import static org.apache.hadoop.ozone.om.request.file.OMFileRequest.OMDirectoryResult.FILE_EXISTS_IN_GIVENPATH; import static org.apache.hadoop.ozone.om.request.file.OMFileRequest.OMDirectoryResult.NONE; @@ -139,12 +140,14 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName, bucketName); - // TODO: Not checking volume exist here, once we have full cache we can - // add volume exist check also. + // Check volume exist. + if (omMetadataManager.getVolumeTable().isExist(volumeName)) { + throw new OMException("Volume not found " + volumeName, + VOLUME_NOT_FOUND); + } OmBucketInfo omBucketInfo = omMetadataManager.getBucketTable().get( omMetadataManager.getBucketKey(volumeName, bucketName)); - if (omBucketInfo == null) { throw new OMException("Bucket not found " + bucketName, BUCKET_NOT_FOUND); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java index ee4b9b2dc0dd..a81bb8d114f6 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java @@ -49,6 +49,10 @@ import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes .KEY_NOT_FOUND; +import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes + .BUCKET_NOT_FOUND; +import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes + .VOLUME_NOT_FOUND; import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK; /** @@ -117,12 +121,19 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName, bucketName); - // Not doing bucket/volume checks here. In this way we can avoid db - // checks for them. - // TODO: Once we have volume/bucket full cache, we can add - // them back, as these checks will be inexpensive at that time. - OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey); + // Check volume exist. + if (omMetadataManager.getVolumeTable().isExist(volumeName)) { + throw new OMException("Volume not found " + volumeName, + VOLUME_NOT_FOUND); + } + // Check bucket exist. + if (omMetadataManager.getBucketTable().isExist(bucketName)) { + throw new OMException("Bucket not found " + bucketName, + BUCKET_NOT_FOUND); + } + + OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey); if (omKeyInfo == null) { throw new OMException("Key not found", KEY_NOT_FOUND); } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java index 526473c23997..744b00be3833 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java @@ -50,6 +50,9 @@ import org.apache.hadoop.hdds.utils.db.cache.CacheValue; import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.KEY_NOT_FOUND; +import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.BUCKET_NOT_FOUND; +import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.VOLUME_NOT_FOUND; + import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK; /** @@ -123,10 +126,17 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName, bucketName); - // Not doing bucket/volume checks here. In this way we can avoid db - // checks for them. - // TODO: Once we have volume/bucket full cache, we can add - // them back, as these checks will be inexpensive at that time. + // Check volume exist. + if (omMetadataManager.getVolumeTable().isExist(volumeName)) { + throw new OMException("Volume not found " + volumeName, + VOLUME_NOT_FOUND); + } + + // Check bucket exist. + if (omMetadataManager.getBucketTable().isExist(bucketName)) { + throw new OMException("Bucket not found " + bucketName, + BUCKET_NOT_FOUND); + } // fromKeyName should exist String fromKey = omMetadataManager.getOzoneKey( From 992fc8bd3663a34ab7b7f89fbf9397a95b102116 Mon Sep 17 00:00:00 2001 From: cxorm Date: Wed, 9 Oct 2019 00:22:12 +0800 Subject: [PATCH 2/5] Revert "HDDS-1737. Add Volume check in KeyManager and File Operations." This reverts commit 369e206a3b6505013fa8b4933cd89bc6a8a4c27c. --- .../file/OMDirectoryCreateRequest.java | 11 ++++------ .../om/request/key/OMKeyDeleteRequest.java | 21 +++++-------------- .../om/request/key/OMKeyRenameRequest.java | 18 ++++------------ 3 files changed, 13 insertions(+), 37 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java index 3cb74f9a88c2..4b591dbed2d5 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java @@ -63,9 +63,8 @@ import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.BUCKET_NOT_FOUND; -import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.VOLUME_NOT_FOUND; import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.FILE_ALREADY_EXISTS; -import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK; +import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK; import static org.apache.hadoop.ozone.om.request.file.OMFileRequest.OMDirectoryResult.DIRECTORY_EXISTS_IN_GIVENPATH; import static org.apache.hadoop.ozone.om.request.file.OMFileRequest.OMDirectoryResult.FILE_EXISTS_IN_GIVENPATH; import static org.apache.hadoop.ozone.om.request.file.OMFileRequest.OMDirectoryResult.NONE; @@ -140,14 +139,12 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName, bucketName); - // Check volume exist. - if (omMetadataManager.getVolumeTable().isExist(volumeName)) { - throw new OMException("Volume not found " + volumeName, - VOLUME_NOT_FOUND); - } + // TODO: Not checking volume exist here, once we have full cache we can + // add volume exist check also. OmBucketInfo omBucketInfo = omMetadataManager.getBucketTable().get( omMetadataManager.getBucketKey(volumeName, bucketName)); + if (omBucketInfo == null) { throw new OMException("Bucket not found " + bucketName, BUCKET_NOT_FOUND); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java index a81bb8d114f6..ee4b9b2dc0dd 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java @@ -49,10 +49,6 @@ import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes .KEY_NOT_FOUND; -import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes - .BUCKET_NOT_FOUND; -import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes - .VOLUME_NOT_FOUND; import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK; /** @@ -121,19 +117,12 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName, bucketName); - // Check volume exist. - if (omMetadataManager.getVolumeTable().isExist(volumeName)) { - throw new OMException("Volume not found " + volumeName, - VOLUME_NOT_FOUND); - } - - // Check bucket exist. - if (omMetadataManager.getBucketTable().isExist(bucketName)) { - throw new OMException("Bucket not found " + bucketName, - BUCKET_NOT_FOUND); - } - + // Not doing bucket/volume checks here. In this way we can avoid db + // checks for them. + // TODO: Once we have volume/bucket full cache, we can add + // them back, as these checks will be inexpensive at that time. OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey); + if (omKeyInfo == null) { throw new OMException("Key not found", KEY_NOT_FOUND); } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java index 744b00be3833..526473c23997 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java @@ -50,9 +50,6 @@ import org.apache.hadoop.hdds.utils.db.cache.CacheValue; import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.KEY_NOT_FOUND; -import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.BUCKET_NOT_FOUND; -import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.VOLUME_NOT_FOUND; - import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK; /** @@ -126,17 +123,10 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName, bucketName); - // Check volume exist. - if (omMetadataManager.getVolumeTable().isExist(volumeName)) { - throw new OMException("Volume not found " + volumeName, - VOLUME_NOT_FOUND); - } - - // Check bucket exist. - if (omMetadataManager.getBucketTable().isExist(bucketName)) { - throw new OMException("Bucket not found " + bucketName, - BUCKET_NOT_FOUND); - } + // Not doing bucket/volume checks here. In this way we can avoid db + // checks for them. + // TODO: Once we have volume/bucket full cache, we can add + // them back, as these checks will be inexpensive at that time. // fromKeyName should exist String fromKey = omMetadataManager.getOzoneKey( From f3faf74aa3d9faa8d05f943a023525388548795d Mon Sep 17 00:00:00 2001 From: cxorm Date: Thu, 10 Oct 2019 16:11:55 +0800 Subject: [PATCH 3/5] Add tests aiming to modify requests with full cache Add some tests for the fixing of request. --- .../file/TestOMDirectoryCreateRequest.java | 34 ++++++++++++++++++- .../request/file/TestOMFileCreateRequest.java | 21 ++++++++++++ .../request/key/TestOMKeyDeleteRequest.java | 26 ++++++++++---- .../request/key/TestOMKeyRenameRequest.java | 28 ++++++++++++--- 4 files changed, 97 insertions(+), 12 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMDirectoryCreateRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMDirectoryCreateRequest.java index 4e93b13ad498..c0b1f00a9eae 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMDirectoryCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMDirectoryCreateRequest.java @@ -152,6 +152,38 @@ public void testValidateAndUpdateCache() throws Exception { } + @Test + public void testValidateAndUpdateCacheWithVolumeNotFound() throws Exception { + String volumeName = "vol1"; + String bucketName = "bucket1"; + String keyName = RandomStringUtils.randomAlphabetic(5); + for (int i =0; i< 3; i++) { + keyName += "/" + RandomStringUtils.randomAlphabetic(5); + } + + OMRequest omRequest = createDirectoryRequest(volumeName, bucketName, + keyName); + OMDirectoryCreateRequest omDirectoryCreateRequest = + new OMDirectoryCreateRequest(omRequest); + + OMRequest modifiedOmRequest = + omDirectoryCreateRequest.preExecute(ozoneManager); + + omDirectoryCreateRequest = new OMDirectoryCreateRequest(modifiedOmRequest); + + OMClientResponse omClientResponse = + omDirectoryCreateRequest.validateAndUpdateCache(ozoneManager, 100L, + ozoneManagerDoubleBufferHelper); + + Assert.assertTrue(omClientResponse.getOMResponse().getStatus() + == OzoneManagerProtocolProtos.Status.VOLUME_NOT_FOUND); + + // Key should not exist in DB + Assert.assertTrue(omMetadataManager.getKeyTable().get( + omMetadataManager.getOzoneDirKey( + volumeName, bucketName, keyName)) == null); + + } @Test public void testValidateAndUpdateCacheWithBucketNotFound() throws Exception { @@ -171,6 +203,7 @@ public void testValidateAndUpdateCacheWithBucketNotFound() throws Exception { omDirectoryCreateRequest.preExecute(ozoneManager); omDirectoryCreateRequest = new OMDirectoryCreateRequest(modifiedOmRequest); + TestOMRequestUtils.addVolumeToDB(volumeName, omMetadataManager); OMClientResponse omClientResponse = omDirectoryCreateRequest.validateAndUpdateCache(ozoneManager, 100L, @@ -183,7 +216,6 @@ public void testValidateAndUpdateCacheWithBucketNotFound() throws Exception { Assert.assertTrue(omMetadataManager.getKeyTable().get( omMetadataManager.getOzoneDirKey( volumeName, bucketName, keyName)) == null); - } @Test diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMFileCreateRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMFileCreateRequest.java index 9639af03e595..93527a19ab0c 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMFileCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMFileCreateRequest.java @@ -38,6 +38,7 @@ import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos .OMRequest; +import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.VOLUME_NOT_FOUND; import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.BUCKET_NOT_FOUND; import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.FILE_ALREADY_EXISTS; import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.NOT_A_FILE; @@ -177,6 +178,26 @@ public void testValidateAndUpdateCache() throws Exception { } + @Test + public void testValidateAndUpdateCacheWithVolumeNotFound() throws Exception { + OMRequest omRequest = createFileRequest(volumeName, bucketName, keyName, + HddsProtos.ReplicationFactor.ONE, HddsProtos.ReplicationType.RATIS, + false, true); + + OMFileCreateRequest omFileCreateRequest = new OMFileCreateRequest( + omRequest); + + OMRequest modifiedOmRequest = omFileCreateRequest.preExecute(ozoneManager); + + omFileCreateRequest = new OMFileCreateRequest(modifiedOmRequest); + + OMClientResponse omFileCreateResponse = + omFileCreateRequest.validateAndUpdateCache(ozoneManager, 100L, + ozoneManagerDoubleBufferHelper); + Assert.assertEquals(VOLUME_NOT_FOUND, + omFileCreateResponse.getOMResponse().getStatus()); + + } @Test public void testValidateAndUpdateCacheWithBucketNotFound() throws Exception { diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequest.java index e95ecd54396e..94e810dd90e2 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequest.java @@ -107,26 +107,40 @@ public void testValidateAndUpdateCacheWithKeyNotFound() throws Exception { omClientResponse.getOMResponse().getStatus()); } + @Test - public void testValidateAndUpdateCacheWithOutVolumeAndBucket() - throws Exception { + public void testValidateAndUpdateCacheWithVolumeNotFound() throws Exception { OMRequest modifiedOmRequest = doPreExecute(createDeleteKeyRequest()); OMKeyDeleteRequest omKeyDeleteRequest = new OMKeyDeleteRequest(modifiedOmRequest); - // In actual implementation we don't check for bucket/volume exists - // during delete key. So it should still return error KEY_NOT_FOUND - OMClientResponse omClientResponse = omKeyDeleteRequest.validateAndUpdateCache(ozoneManager, 100L, ozoneManagerDoubleBufferHelper); - Assert.assertEquals(OzoneManagerProtocolProtos.Status.KEY_NOT_FOUND, + Assert.assertEquals(OzoneManagerProtocolProtos.Status.VOLUME_NOT_FOUND, omClientResponse.getOMResponse().getStatus()); } + @Test + public void testValidateAndUpdateCacheWithBucketNotFound() throws Exception { + OMRequest modifiedOmRequest = + doPreExecute(createDeleteKeyRequest()); + + OMKeyDeleteRequest omKeyDeleteRequest = + new OMKeyDeleteRequest(modifiedOmRequest); + + TestOMRequestUtils.addVolumeToDB(volumeName, omMetadataManager); + + OMClientResponse omClientResponse = + omKeyDeleteRequest.validateAndUpdateCache(ozoneManager, + 100L, ozoneManagerDoubleBufferHelper); + + Assert.assertEquals(OzoneManagerProtocolProtos.Status.BUCKET_NOT_FOUND, + omClientResponse.getOMResponse().getStatus()); + } /** * This method calls preExecute and verify the modified request. diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequest.java index 864ba06111ac..57b1c9cc5b46 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequest.java @@ -49,6 +49,8 @@ public void testValidateAndUpdateCache() throws Exception { OMRequest modifiedOmRequest = doPreExecute(createRenameKeyRequest(toKeyName)); + TestOMRequestUtils.addVolumeAndBucketToDB(volumeName, bucketName, + omMetadataManager); TestOMRequestUtils.addKeyToTable(false, volumeName, bucketName, keyName, clientID, replicationType, replicationFactor, omMetadataManager); @@ -112,16 +114,32 @@ public void testValidateAndUpdateCacheWithKeyNotFound() throws Exception { } + @Test + public void testValidateAndUpdateCacheWithVolumeNotFound() throws Exception { + String toKeyName = UUID.randomUUID().toString(); + OMRequest modifiedOmRequest = + doPreExecute(createRenameKeyRequest(toKeyName)); + + OMKeyRenameRequest omKeyRenameRequest = + new OMKeyRenameRequest(modifiedOmRequest); + + OMClientResponse omKeyRenameResponse = + omKeyRenameRequest.validateAndUpdateCache(ozoneManager, 100L, + ozoneManagerDoubleBufferHelper); + + Assert.assertEquals(OzoneManagerProtocolProtos.Status.VOLUME_NOT_FOUND, + omKeyRenameResponse.getOMResponse().getStatus()); + + } @Test - public void testValidateAndUpdateCacheWithOutVolumeAndBucket() - throws Exception { + public void testValidateAndUpdateCacheWithBucketNotFound() throws Exception { String toKeyName = UUID.randomUUID().toString(); OMRequest modifiedOmRequest = doPreExecute(createRenameKeyRequest(toKeyName)); - // In actual implementation we don't check for bucket/volume exists - // during delete key. So it should still return error KEY_NOT_FOUND + // Add only volume entry to DB. + TestOMRequestUtils.addVolumeToDB(volumeName, omMetadataManager); OMKeyRenameRequest omKeyRenameRequest = new OMKeyRenameRequest(modifiedOmRequest); @@ -130,7 +148,7 @@ public void testValidateAndUpdateCacheWithOutVolumeAndBucket() omKeyRenameRequest.validateAndUpdateCache(ozoneManager, 100L, ozoneManagerDoubleBufferHelper); - Assert.assertEquals(OzoneManagerProtocolProtos.Status.KEY_NOT_FOUND, + Assert.assertEquals(OzoneManagerProtocolProtos.Status.BUCKET_NOT_FOUND, omKeyRenameResponse.getOMResponse().getStatus()); } From 322b3c633f8b48e81a7dc1b8334c0d7fee503f7d Mon Sep 17 00:00:00 2001 From: cxorm Date: Thu, 10 Oct 2019 16:16:45 +0800 Subject: [PATCH 4/5] HDDS-1737. Add Volume check in KeyManager and File Operations. --- .../request/file/OMDirectoryCreateRequest.java | 16 ++++------------ .../om/request/file/OMFileCreateRequest.java | 11 +++-------- .../ozone/om/request/key/OMKeyDeleteRequest.java | 11 ++++------- .../ozone/om/request/key/OMKeyRenameRequest.java | 6 ++---- 4 files changed, 13 insertions(+), 31 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java index 4b591dbed2d5..9b451e9c569f 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java @@ -62,9 +62,8 @@ import org.apache.hadoop.hdds.utils.db.cache.CacheValue; -import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.BUCKET_NOT_FOUND; import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.FILE_ALREADY_EXISTS; -import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK; +import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK; import static org.apache.hadoop.ozone.om.request.file.OMFileRequest.OMDirectoryResult.DIRECTORY_EXISTS_IN_GIVENPATH; import static org.apache.hadoop.ozone.om.request.file.OMFileRequest.OMDirectoryResult.FILE_EXISTS_IN_GIVENPATH; import static org.apache.hadoop.ozone.om.request.file.OMFileRequest.OMDirectoryResult.NONE; @@ -139,16 +138,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName, bucketName); - // TODO: Not checking volume exist here, once we have full cache we can - // add volume exist check also. - - OmBucketInfo omBucketInfo = omMetadataManager.getBucketTable().get( - omMetadataManager.getBucketKey(volumeName, bucketName)); - - if (omBucketInfo == null) { - throw new OMException("Bucket not found " + bucketName, - BUCKET_NOT_FOUND); - } + validateBucketAndVolume(omMetadataManager, volumeName, bucketName); // Need to check if any files exist in the given path, if they exist we // cannot create a directory with the given key. @@ -156,6 +146,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, OMFileRequest.verifyFilesInPath(omMetadataManager, volumeName, bucketName, keyName, Paths.get(keyName)); + OmBucketInfo omBucketInfo = omMetadataManager.getBucketTable().get( + omMetadataManager.getBucketKey(volumeName, bucketName)); OmKeyInfo dirKeyInfo = null; if (omDirectoryResult == FILE_EXISTS || omDirectoryResult == FILE_EXISTS_IN_GIVENPATH) { diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequest.java index 20b51747caaa..98547c066b6e 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequest.java @@ -183,14 +183,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName, bucketName); - OmBucketInfo bucketInfo = - omMetadataManager.getBucketTable().get( - omMetadataManager.getBucketKey(volumeName, bucketName)); - - if (bucketInfo == null) { - throw new OMException("Bucket " + bucketName + " not found", - OMException.ResultCodes.BUCKET_NOT_FOUND); - } + validateBucketAndVolume(omMetadataManager, volumeName, bucketName); if (keyName.length() == 0) { // Check if this is the root of the filesystem. @@ -255,6 +248,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, } // do open key + OmBucketInfo bucketInfo = omMetadataManager.getBucketTable().get( + omMetadataManager.getBucketKey(volumeName, bucketName)); encryptionInfo = getFileEncryptionInfo(ozoneManager, bucketInfo); omKeyInfo = prepareKeyInfo(omMetadataManager, keyArgs, omMetadataManager.getOzoneKey(volumeName, bucketName, diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java index ee4b9b2dc0dd..9a0b7938599b 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java @@ -47,8 +47,7 @@ import org.apache.hadoop.hdds.utils.db.cache.CacheKey; import org.apache.hadoop.hdds.utils.db.cache.CacheValue; -import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes - .KEY_NOT_FOUND; +import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.KEY_NOT_FOUND; import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK; /** @@ -117,12 +116,10 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName, bucketName); - // Not doing bucket/volume checks here. In this way we can avoid db - // checks for them. - // TODO: Once we have volume/bucket full cache, we can add - // them back, as these checks will be inexpensive at that time. - OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey); + // Validate bucket and volume exists or not. + validateBucketAndVolume(omMetadataManager, volumeName, bucketName); + OmKeyInfo omKeyInfo = omMetadataManager.getKeyTable().get(objectKey); if (omKeyInfo == null) { throw new OMException("Key not found", KEY_NOT_FOUND); } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java index 526473c23997..3ae260625677 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java @@ -123,10 +123,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, acquiredLock = omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName, bucketName); - // Not doing bucket/volume checks here. In this way we can avoid db - // checks for them. - // TODO: Once we have volume/bucket full cache, we can add - // them back, as these checks will be inexpensive at that time. + // Validate bucket and volume exists or not. + validateBucketAndVolume(omMetadataManager, volumeName, bucketName); // fromKeyName should exist String fromKey = omMetadataManager.getOzoneKey( From 6d7157a8b750a6a6c08056e2d45dd2c3f7cb4875 Mon Sep 17 00:00:00 2001 From: cxorm Date: Wed, 16 Oct 2019 14:33:00 +0800 Subject: [PATCH 5/5] Modify assert method in TestOMDirectoryCreateRequest.java --- .../om/request/file/TestOMDirectoryCreateRequest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMDirectoryCreateRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMDirectoryCreateRequest.java index c0b1f00a9eae..ba9253dc97fe 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMDirectoryCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMDirectoryCreateRequest.java @@ -53,6 +53,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.when; +import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.VOLUME_NOT_FOUND; /** * Test OM directory create request. @@ -175,13 +176,12 @@ public void testValidateAndUpdateCacheWithVolumeNotFound() throws Exception { omDirectoryCreateRequest.validateAndUpdateCache(ozoneManager, 100L, ozoneManagerDoubleBufferHelper); - Assert.assertTrue(omClientResponse.getOMResponse().getStatus() - == OzoneManagerProtocolProtos.Status.VOLUME_NOT_FOUND); + Assert.assertEquals(VOLUME_NOT_FOUND, + omClientResponse.getOMResponse().getStatus()); // Key should not exist in DB - Assert.assertTrue(omMetadataManager.getKeyTable().get( - omMetadataManager.getOzoneDirKey( - volumeName, bucketName, keyName)) == null); + Assert.assertNull(omMetadataManager.getKeyTable(). + get(omMetadataManager.getOzoneDirKey(volumeName, bucketName, keyName))); }