From 447033985e107716e9631379e19c2bfbe9add968 Mon Sep 17 00:00:00 2001 From: Charles Allen Date: Thu, 2 Jun 2016 09:27:21 -0700 Subject: [PATCH] Make S3DataSegmentMover not bother checking for items if they are the same (#3032) * Make S3DataSegmentMover not bother checking for items if they are the same --- .../druid/storage/s3/S3DataSegmentMover.java | 84 ++++++++++--------- .../storage/s3/S3DataSegmentMoverTest.java | 46 ++++++++++ 2 files changed, 89 insertions(+), 41 deletions(-) diff --git a/extensions-core/s3-extensions/src/main/java/io/druid/storage/s3/S3DataSegmentMover.java b/extensions-core/s3-extensions/src/main/java/io/druid/storage/s3/S3DataSegmentMover.java index 5d23db3d950..f89f7a7c9c1 100644 --- a/extensions-core/s3-extensions/src/main/java/io/druid/storage/s3/S3DataSegmentMover.java +++ b/extensions-core/s3-extensions/src/main/java/io/druid/storage/s3/S3DataSegmentMover.java @@ -82,21 +82,21 @@ public class S3DataSegmentMover implements DataSegmentMover return segment.withLoadSpec( ImmutableMap.builder() - .putAll( - Maps.filterKeys( - loadSpec, new Predicate() - { - @Override - public boolean apply(String input) - { - return !(input.equals("bucket") || input.equals("key")); - } - } - ) - ) - .put("bucket", targetS3Bucket) - .put("key", targetS3Path) - .build() + .putAll( + Maps.filterKeys( + loadSpec, new Predicate() + { + @Override + public boolean apply(String input) + { + return !(input.equals("bucket") || input.equals("key")); + } + } + ) + ) + .put("bucket", targetS3Bucket) + .put("key", targetS3Path) + .build() ); } catch (ServiceException e) { @@ -118,33 +118,33 @@ public class S3DataSegmentMover implements DataSegmentMover @Override public Void call() throws Exception { + if (s3Bucket.equals(targetS3Bucket) && s3Path.equals(targetS3Path)) { + log.info("No need to move file[s3://%s/%s] onto itself", s3Bucket, s3Path); + return null; + } if (s3Client.isObjectInBucket(s3Bucket, s3Path)) { - if (s3Bucket.equals(targetS3Bucket) && s3Path.equals(targetS3Path)) { - log.info("No need to move file[s3://%s/%s] onto itself", s3Bucket, s3Path); + final S3Object[] list = s3Client.listObjects(s3Bucket, s3Path, ""); + if (list.length == 0) { + // should never happen + throw new ISE("Unable to list object [s3://%s/%s]", s3Bucket, s3Path); + } + final S3Object s3Object = list[0]; + if (s3Object.getStorageClass() != null && + s3Object.getStorageClass().equals(S3Object.STORAGE_CLASS_GLACIER)) { + log.warn("Cannot move file[s3://%s/%s] of storage class glacier, skipping.", s3Bucket, s3Path); } else { - final S3Object[] list = s3Client.listObjects(s3Bucket, s3Path, ""); - if (list.length == 0) { - // should never happen - throw new ISE("Unable to list object [s3://%s/%s]", s3Bucket, s3Path); - } - final S3Object s3Object = list[0]; - if (s3Object.getStorageClass() != null && - s3Object.getStorageClass().equals(S3Object.STORAGE_CLASS_GLACIER)) { - log.warn("Cannot move file[s3://%s/%s] of storage class glacier, skipping.", s3Bucket, s3Path); - } else { - log.info( - "Moving file[s3://%s/%s] to [s3://%s/%s]", - s3Bucket, - s3Path, - targetS3Bucket, - targetS3Path - ); - final S3Object target = new S3Object(targetS3Path); - if (!config.getDisableAcl()) { - target.setAcl(GSAccessControlList.REST_CANNED_BUCKET_OWNER_FULL_CONTROL); - } - s3Client.moveObject(s3Bucket, s3Path, targetS3Bucket, target, false); + log.info( + "Moving file[s3://%s/%s] to [s3://%s/%s]", + s3Bucket, + s3Path, + targetS3Bucket, + targetS3Path + ); + final S3Object target = new S3Object(targetS3Path); + if (!config.getDisableAcl()) { + target.setAcl(GSAccessControlList.REST_CANNED_BUCKET_OWNER_FULL_CONTROL); } + s3Client.moveObject(s3Bucket, s3Path, targetS3Bucket, target, false); } } else { // ensure object exists in target location @@ -157,8 +157,10 @@ public class S3DataSegmentMover implements DataSegmentMover } else { throw new SegmentLoadingException( "Unable to move file [s3://%s/%s] to [s3://%s/%s], not present in either source or target location", - s3Bucket, s3Path, - targetS3Bucket, targetS3Path + s3Bucket, + s3Path, + targetS3Bucket, + targetS3Path ); } } diff --git a/extensions-core/s3-extensions/src/test/java/io/druid/storage/s3/S3DataSegmentMoverTest.java b/extensions-core/s3-extensions/src/test/java/io/druid/storage/s3/S3DataSegmentMoverTest.java index 93dc2b44de1..e145f896f2c 100644 --- a/extensions-core/s3-extensions/src/test/java/io/druid/storage/s3/S3DataSegmentMoverTest.java +++ b/extensions-core/s3-extensions/src/test/java/io/druid/storage/s3/S3DataSegmentMoverTest.java @@ -110,6 +110,52 @@ public class S3DataSegmentMoverTest ImmutableMap.of("baseKey", "targetBaseKey", "bucket", "archive") ); } + + @Test + public void testIgnoresGoneButAlreadyMoved() throws Exception + { + MockStorageService mockS3Client = new MockStorageService(); + S3DataSegmentMover mover = new S3DataSegmentMover(mockS3Client, new S3DataSegmentPusherConfig()); + mover.move(new DataSegment( + "test", + new Interval("2013-01-01/2013-01-02"), + "1", + ImmutableMap.of( + "key", + "baseKey/test/2013-01-01T00:00:00.000Z_2013-01-02T00:00:00.000Z/1/0/index.zip", + "bucket", + "DOES NOT EXIST" + ), + ImmutableList.of("dim1", "dim1"), + ImmutableList.of("metric1", "metric2"), + new NoneShardSpec(), + 0, + 1 + ), ImmutableMap.of("bucket", "DOES NOT EXIST", "baseKey", "baseKey")); + } + + @Test(expected = SegmentLoadingException.class) + public void testFailsToMoveMissing() throws Exception + { + MockStorageService mockS3Client = new MockStorageService(); + S3DataSegmentMover mover = new S3DataSegmentMover(mockS3Client, new S3DataSegmentPusherConfig()); + mover.move(new DataSegment( + "test", + new Interval("2013-01-01/2013-01-02"), + "1", + ImmutableMap.of( + "key", + "baseKey/test/2013-01-01T00:00:00.000Z_2013-01-02T00:00:00.000Z/1/0/index.zip", + "bucket", + "DOES NOT EXIST" + ), + ImmutableList.of("dim1", "dim1"), + ImmutableList.of("metric1", "metric2"), + new NoneShardSpec(), + 0, + 1 + ), ImmutableMap.of("bucket", "DOES NOT EXIST", "baseKey", "baseKey2")); + } private class MockStorageService extends RestS3Service { Map> storage = Maps.newHashMap();