You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/03/05 08:10:54 UTC
[GitHub] [druid] zachjsh opened a new pull request #9459: Ability to Delete
task logs and segments from S3
zachjsh opened a new pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459
* implement ability to delete all tasks logs or all task logs
written before a particular date when written to S3
* implement ability to delete all segments from S3 deep storage
* upgrade version of aws SDK in use
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist above are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
<hr>
##### Key changed/added classes in this PR
* `MyFoo`
* `OurBar`
* `TheirBaz`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r390039245
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentKiller.java
##########
@@ -69,8 +85,48 @@ public void kill(DataSegment segment) throws SegmentLoadingException
}
@Override
- public void killAll()
+ public void killAll() throws IOException
{
- throw new UnsupportedOperationException("not implemented");
+ try {
+ S3Utils.retryS3Operation(
+ () -> {
+ String bucketName = segmentPusherConfig.getBucket();
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r388682379
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentKiller.java
##########
@@ -69,8 +85,48 @@ public void kill(DataSegment segment) throws SegmentLoadingException
}
@Override
- public void killAll()
+ public void killAll() throws IOException
{
- throw new UnsupportedOperationException("not implemented");
+ try {
+ S3Utils.retryS3Operation(
+ () -> {
+ String bucketName = segmentPusherConfig.getBucket();
Review comment:
Hmm, this block is almost identical to the block in `S3TaskLogs.killOlderThan`, and both partially very close to what `ObjectSummaryIterator` is providing.
I think it would be nicer if this class and `S3TaskLogs` could use `ObjectSummaryIterator`, and if the method that does the bulk key delete can be put in a shared method in `S3Utils`.
Can I recommend something like this in `S3Utils`?
```java
public static void deleteObjectsInPath(
ServerSideEncryptingAmazonS3 s3Client,
S3InputDataConfig config,
String bucket,
String prefix,
Predicate<S3ObjectSummary> filter
)
throws Exception
{
final List<DeleteObjectsRequest.KeyVersion> keysToDelete = new ArrayList<>(config.getMaxListingLength());
final ObjectSummaryIterator iterator = new ObjectSummaryIterator(
s3Client,
ImmutableList.of(new CloudObjectLocation(bucket, prefix).toUri("s3")),
config.getMaxListingLength()
);
while (iterator.hasNext()) {
final S3ObjectSummary nextObject = iterator.next();
if (filter.apply(nextObject)) {
keysToDelete.add(new DeleteObjectsRequest.KeyVersion(nextObject.getKey()));
if (keysToDelete.size() == config.getMaxListingLength()) {
deleteBucketKeys(s3Client, bucket, keysToDelete);
keysToDelete.clear();
}
}
}
if (keysToDelete.size() > 0) {
deleteBucketKeys(s3Client, bucket, keysToDelete);
}
}
public static void deleteBucketKeys(
ServerSideEncryptingAmazonS3 s3Client,
String bucket,
List<DeleteObjectsRequest.KeyVersion> keysToDelete
)
throws Exception
{
DeleteObjectsRequest deleteRequest = new DeleteObjectsRequest(bucket).withKeys(keysToDelete);
S3Utils.retryS3Operation(() -> {
s3Client.deleteObjects(deleteRequest);
return null;
});
}
```
Then, not only does it share all the code for listing objects, it also pushes down the retry to the specific API calls to list and delete, instead of wrapping the entire loop, which I think is better.
If you make a change like this, then the kill calls become something like:
```java
@Override
public void killAll() throws IOException
{
try {
S3Utils.deleteObjectsInPath(
s3Client,
inputDataConfig,
segmentPusherConfig.getBucket(),
segmentPusherConfig.getBaseKey(),
Predicates.alwaysTrue()
);
}
catch (Exception e) {
log.error("Error occurred while deleting segment files from s3. Error: %s", e.getMessage());
throw new IOException(e);
}
}
```
and
```java
@Override
public void killOlderThan(long timestamp) throws IOException
{
try {
S3Utils.deleteObjectsInPath(
service,
inputDataConfig,
config.getS3Bucket(),
config.getS3Prefix(),
(object) -> object.getLastModified().getTime() < timestamp
);
}
catch (Exception e) {
log.error("Error occurred while deleting task log files from s3. Error: %s", e.getMessage());
throw new IOException(e);
}
}
```
`ObjectSummaryIterator` currently will skip things it thinks are directories, if this is _not_ desirable for deleting segments and task logs, then I would suggest maybe pushing down the predicate to the `ObjectSummaryIterator` so that the existing users can filter out directories, and your usages can get everything or filter by timestamp as appropriate. In addition, if it shouldn't skip directories, it would probably be worth adding a test for what happens when one is present in the list objects response, if not present.
disclaimer: I didn't test this in real s3, but did run unit tests which passed after some modifications to expected calls, but would still recommend to review these snippets first to make sure they are correct.
What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r390039337
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentKiller.java
##########
@@ -69,8 +85,48 @@ public void kill(DataSegment segment) throws SegmentLoadingException
}
@Override
- public void killAll()
+ public void killAll() throws IOException
{
- throw new UnsupportedOperationException("not implemented");
+ try {
+ S3Utils.retryS3Operation(
+ () -> {
+ String bucketName = segmentPusherConfig.getBucket();
+ String prefix = segmentPusherConfig.getBaseKey();
+ int maxListingLength = inputDataConfig.getMaxListingLength();
+ ListObjectsV2Result result;
+ String continuationToken = null;
+ do {
+ log.info("Deleting batch of %d segment files from s3 location [bucket: %s prefix: %s].",
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r390039313
##########
File path: core/src/main/java/org/apache/druid/common/utils/TimeSupplier.java
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.druid.common.utils;
+
+import java.util.function.Supplier;
+
+public class TimeSupplier implements Supplier<Long>
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r388807839
##########
File path: core/src/main/java/org/apache/druid/common/utils/TimeSupplier.java
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.druid.common.utils;
+
+import java.util.function.Supplier;
+
+public class TimeSupplier implements Supplier<Long>
Review comment:
You're right that in this case that most of the logic here lives in killOlderThan which killAll delegates to, thought I think that a supplier for current time could be useful in other cases. I will change name and add javadocs to make its purpose more clear. You didn't miss the binding, there is none. This works since it is not an interface. Should I make an interface for this and explicitly bind this implementation to the interface? What would be a good module to do this in?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r390538553
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3Utils.java
##########
@@ -200,6 +204,54 @@ public static S3ObjectSummary getSingleObjectSummary(ServerSideEncryptingAmazonS
return objectSummary;
}
+ public static void deleteObjectsInPath(
+ ServerSideEncryptingAmazonS3 s3Client,
+ S3InputDataConfig config,
+ String bucket,
+ String prefix,
+ Predicate<S3ObjectSummary> filter
+ )
+ throws Exception
+ {
+ final List<DeleteObjectsRequest.KeyVersion> keysToDelete = new ArrayList<>(config.getMaxListingLength());
+ final ObjectSummaryIterator iterator = new ObjectSummaryIterator(
+ s3Client,
+ ImmutableList.of(new CloudObjectLocation(bucket, prefix).toUri("s3")),
+ config.getMaxListingLength()
+ );
+
+ while (iterator.hasNext()) {
+ final S3ObjectSummary nextObject = iterator.next();
+ if (filter.apply(nextObject)) {
+ keysToDelete.add(new DeleteObjectsRequest.KeyVersion(nextObject.getKey()));
+ if (keysToDelete.size() == config.getMaxListingLength()) {
+ deleteBucketKeys(s3Client, bucket, keysToDelete);
+ log.info("Deleted %d files", keysToDelete.size());
+ keysToDelete.clear();
+ }
+ }
+ }
+
+ if (keysToDelete.size() > 0) {
+ deleteBucketKeys(s3Client, bucket, keysToDelete);
+ log.info("Deleted %d files", keysToDelete.size());
+ }
+ }
+
+ public static void deleteBucketKeys(
Review comment:
I'll get this in the next change which should be coming shortly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r389184625
##########
File path: core/src/main/java/org/apache/druid/common/utils/TimeSupplier.java
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.druid.common.utils;
+
+import java.util.function.Supplier;
+
+public class TimeSupplier implements Supplier<Long>
Review comment:
>You didn't miss the binding, there is none. This works since it is not an interface. Should I make an interface for this and explicitly bind this implementation to the interface? What would be a good module to do this in?
Oh yeah, my bad, I'm not a guice wizard and forget how things work sometimes 😜. No reason to make an interface if only one implementation i think as long as everything works :+1:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r390066428
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3Utils.java
##########
@@ -200,6 +204,54 @@ public static S3ObjectSummary getSingleObjectSummary(ServerSideEncryptingAmazonS
return objectSummary;
}
+ public static void deleteObjectsInPath(
+ ServerSideEncryptingAmazonS3 s3Client,
+ S3InputDataConfig config,
+ String bucket,
+ String prefix,
+ Predicate<S3ObjectSummary> filter
+ )
+ throws Exception
+ {
+ final List<DeleteObjectsRequest.KeyVersion> keysToDelete = new ArrayList<>(config.getMaxListingLength());
+ final ObjectSummaryIterator iterator = new ObjectSummaryIterator(
+ s3Client,
+ ImmutableList.of(new CloudObjectLocation(bucket, prefix).toUri("s3")),
+ config.getMaxListingLength()
+ );
+
+ while (iterator.hasNext()) {
+ final S3ObjectSummary nextObject = iterator.next();
+ if (filter.apply(nextObject)) {
+ keysToDelete.add(new DeleteObjectsRequest.KeyVersion(nextObject.getKey()));
+ if (keysToDelete.size() == config.getMaxListingLength()) {
+ deleteBucketKeys(s3Client, bucket, keysToDelete);
+ log.info("Deleted %d files", keysToDelete.size());
+ keysToDelete.clear();
+ }
+ }
+ }
+
+ if (keysToDelete.size() > 0) {
+ deleteBucketKeys(s3Client, bucket, keysToDelete);
+ log.info("Deleted %d files", keysToDelete.size());
+ }
+ }
+
+ public static void deleteBucketKeys(
Review comment:
nit: this can be private actually (my bad)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r390538564
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3Utils.java
##########
@@ -200,6 +204,54 @@ public static S3ObjectSummary getSingleObjectSummary(ServerSideEncryptingAmazonS
return objectSummary;
}
+ public static void deleteObjectsInPath(
Review comment:
I'll get this in the next change which should be coming shortly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r388684988
##########
File path: extensions-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/S3TestUtils.java
##########
@@ -0,0 +1,212 @@
+/*
+ * 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.druid.storage.s3;
+
+import com.amazonaws.services.s3.model.DeleteObjectsRequest;
+import com.amazonaws.services.s3.model.ListObjectsV2Request;
+import com.amazonaws.services.s3.model.ListObjectsV2Result;
+import com.amazonaws.services.s3.model.S3ObjectSummary;
+import junit.framework.AssertionFailedError;
+import org.apache.commons.collections4.map.HashedMap;
+import org.easymock.EasyMock;
+import org.easymock.EasyMockSupport;
+import org.easymock.IArgumentMatcher;
+import org.easymock.IExpectationSetters;
+
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class S3TestUtils extends EasyMockSupport
+{
+ public static ListObjectsV2Request listObjectsV2RequestArgumentMatcher(ListObjectsV2Request listObjectsV2Request)
+ {
+ EasyMock.reportMatcher(new IArgumentMatcher()
+ {
+ @Override
+ public boolean matches(Object argument)
+ {
+
+ return argument instanceof ListObjectsV2Request
+ && listObjectsV2Request.getBucketName().equals(((ListObjectsV2Request) argument).getBucketName())
+ && listObjectsV2Request.getPrefix().equals(((ListObjectsV2Request) argument).getPrefix())
+ && ((listObjectsV2Request.getContinuationToken() == null
+ && ((ListObjectsV2Request) argument).getContinuationToken() == null)
+ || (listObjectsV2Request.getContinuationToken()
+ .equals(((ListObjectsV2Request) argument).getContinuationToken())))
+ && listObjectsV2Request.getMaxKeys().equals(((ListObjectsV2Request) argument).getMaxKeys());
+ }
+
+ @Override
+ public void appendTo(StringBuffer buffer)
+ {
+ String str = "ListObjectsV2Request(\"bucketName:\" \""
+ + listObjectsV2Request.getBucketName()
+ + "\", \"prefix:\""
+ + listObjectsV2Request.getPrefix()
+ + "\", \"continuationToken:\""
+ + listObjectsV2Request.getContinuationToken()
+ + "\", \"maxKeys:\""
+ + listObjectsV2Request.getMaxKeys()
+ + "\")";
+ buffer.append(str);
+ }
+ });
+ return null;
+ }
+
+ public static DeleteObjectsRequest deleteObjectsRequestArgumentMatcher(DeleteObjectsRequest deleteObjectsRequest)
+ {
+ EasyMock.reportMatcher(new IArgumentMatcher()
+ {
+ @Override
+ public boolean matches(Object argument)
+ {
+
+ boolean matches = argument instanceof DeleteObjectsRequest
+ && deleteObjectsRequest.getBucketName()
+ .equals(((DeleteObjectsRequest) argument).getBucketName())
+ && deleteObjectsRequest.getKeys().size() == ((DeleteObjectsRequest) argument).getKeys()
+ .size();
+ if (matches) {
+ Map<String, String> expectedKeysAndVersions = deleteObjectsRequest.getKeys().stream().collect(
+ Collectors.toMap(DeleteObjectsRequest.KeyVersion::getKey, x -> {
+ return x.getVersion() == null ? "null" : x.getVersion();
+ }));
+ Map<String, String> actualKeysAndVersions = ((DeleteObjectsRequest) argument).getKeys().stream().collect(
+ Collectors.toMap(DeleteObjectsRequest.KeyVersion::getKey, x -> {
+ return x.getVersion() == null ? "null" : x.getVersion();
+ }));
+ matches = expectedKeysAndVersions.equals(actualKeysAndVersions);
+ }
+ return matches;
+ }
+
+ @Override
+ public void appendTo(StringBuffer buffer)
+ {
+ String str = "DeleteObjectsRequest(\"bucketName:\" \""
+ + deleteObjectsRequest.getBucketName()
+ + "\", \"keys:\""
+ + deleteObjectsRequest.getKeys()
+ + "\")";
+ buffer.append(str);
+ }
+ });
+ return null;
+ }
+
+ public static S3ObjectSummary mockS3ObjectSummary(long lastModified, String key)
+ {
+ S3ObjectSummary objectSummary = EasyMock.createMock(S3ObjectSummary.class);
+ EasyMock.expect(objectSummary.getLastModified()).andReturn(new Date(lastModified));
+ EasyMock.expectLastCall().anyTimes();
+ EasyMock.expect(objectSummary.getKey()).andReturn(key);
+ EasyMock.expectLastCall().anyTimes();
+ return objectSummary;
+ }
+
+ public static ListObjectsV2Request mockRequest(
+ String bucket,
+ String prefix,
+ int maxKeys,
+ String continuationToken
+ )
+ {
+ ListObjectsV2Request request = EasyMock.createMock(ListObjectsV2Request.class);
+ EasyMock.expect(request.getBucketName()).andReturn(bucket);
+ EasyMock.expectLastCall().anyTimes();
Review comment:
I think these `expect`/`expectLastCall` lines can be collapsed into the form of:
```
EasyMock.expect(request.getBucketName()).andReturn(bucket).anyTimes();
```
plus a handful of other places in the tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #9459: Ability to Delete
task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r388805581
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentKiller.java
##########
@@ -69,8 +85,48 @@ public void kill(DataSegment segment) throws SegmentLoadingException
}
@Override
- public void killAll()
+ public void killAll() throws IOException
{
- throw new UnsupportedOperationException("not implemented");
+ try {
+ S3Utils.retryS3Operation(
+ () -> {
+ String bucketName = segmentPusherConfig.getBucket();
Review comment:
Good point about reusing the object iterator, I actually thought about that before, not sure why I decided against. As you said its good that all the listing code is shared in this case. I believe that I would want to skip directories in this case too, so that should be good here as well. Thanks for the suggestion
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r390039283
##########
File path: extensions-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/S3TestUtils.java
##########
@@ -0,0 +1,212 @@
+/*
+ * 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.druid.storage.s3;
+
+import com.amazonaws.services.s3.model.DeleteObjectsRequest;
+import com.amazonaws.services.s3.model.ListObjectsV2Request;
+import com.amazonaws.services.s3.model.ListObjectsV2Result;
+import com.amazonaws.services.s3.model.S3ObjectSummary;
+import junit.framework.AssertionFailedError;
+import org.apache.commons.collections4.map.HashedMap;
+import org.easymock.EasyMock;
+import org.easymock.EasyMockSupport;
+import org.easymock.IArgumentMatcher;
+import org.easymock.IExpectationSetters;
+
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class S3TestUtils extends EasyMockSupport
+{
+ public static ListObjectsV2Request listObjectsV2RequestArgumentMatcher(ListObjectsV2Request listObjectsV2Request)
+ {
+ EasyMock.reportMatcher(new IArgumentMatcher()
+ {
+ @Override
+ public boolean matches(Object argument)
+ {
+
+ return argument instanceof ListObjectsV2Request
+ && listObjectsV2Request.getBucketName().equals(((ListObjectsV2Request) argument).getBucketName())
+ && listObjectsV2Request.getPrefix().equals(((ListObjectsV2Request) argument).getPrefix())
+ && ((listObjectsV2Request.getContinuationToken() == null
+ && ((ListObjectsV2Request) argument).getContinuationToken() == null)
+ || (listObjectsV2Request.getContinuationToken()
+ .equals(((ListObjectsV2Request) argument).getContinuationToken())))
+ && listObjectsV2Request.getMaxKeys().equals(((ListObjectsV2Request) argument).getMaxKeys());
+ }
+
+ @Override
+ public void appendTo(StringBuffer buffer)
+ {
+ String str = "ListObjectsV2Request(\"bucketName:\" \""
+ + listObjectsV2Request.getBucketName()
+ + "\", \"prefix:\""
+ + listObjectsV2Request.getPrefix()
+ + "\", \"continuationToken:\""
+ + listObjectsV2Request.getContinuationToken()
+ + "\", \"maxKeys:\""
+ + listObjectsV2Request.getMaxKeys()
+ + "\")";
+ buffer.append(str);
+ }
+ });
+ return null;
+ }
+
+ public static DeleteObjectsRequest deleteObjectsRequestArgumentMatcher(DeleteObjectsRequest deleteObjectsRequest)
+ {
+ EasyMock.reportMatcher(new IArgumentMatcher()
+ {
+ @Override
+ public boolean matches(Object argument)
+ {
+
+ boolean matches = argument instanceof DeleteObjectsRequest
+ && deleteObjectsRequest.getBucketName()
+ .equals(((DeleteObjectsRequest) argument).getBucketName())
+ && deleteObjectsRequest.getKeys().size() == ((DeleteObjectsRequest) argument).getKeys()
+ .size();
+ if (matches) {
+ Map<String, String> expectedKeysAndVersions = deleteObjectsRequest.getKeys().stream().collect(
+ Collectors.toMap(DeleteObjectsRequest.KeyVersion::getKey, x -> {
+ return x.getVersion() == null ? "null" : x.getVersion();
+ }));
+ Map<String, String> actualKeysAndVersions = ((DeleteObjectsRequest) argument).getKeys().stream().collect(
+ Collectors.toMap(DeleteObjectsRequest.KeyVersion::getKey, x -> {
+ return x.getVersion() == null ? "null" : x.getVersion();
+ }));
+ matches = expectedKeysAndVersions.equals(actualKeysAndVersions);
+ }
+ return matches;
+ }
+
+ @Override
+ public void appendTo(StringBuffer buffer)
+ {
+ String str = "DeleteObjectsRequest(\"bucketName:\" \""
+ + deleteObjectsRequest.getBucketName()
+ + "\", \"keys:\""
+ + deleteObjectsRequest.getKeys()
+ + "\")";
+ buffer.append(str);
+ }
+ });
+ return null;
+ }
+
+ public static S3ObjectSummary mockS3ObjectSummary(long lastModified, String key)
+ {
+ S3ObjectSummary objectSummary = EasyMock.createMock(S3ObjectSummary.class);
+ EasyMock.expect(objectSummary.getLastModified()).andReturn(new Date(lastModified));
+ EasyMock.expectLastCall().anyTimes();
+ EasyMock.expect(objectSummary.getKey()).andReturn(key);
+ EasyMock.expectLastCall().anyTimes();
+ return objectSummary;
+ }
+
+ public static ListObjectsV2Request mockRequest(
+ String bucket,
+ String prefix,
+ int maxKeys,
+ String continuationToken
+ )
+ {
+ ListObjectsV2Request request = EasyMock.createMock(ListObjectsV2Request.class);
+ EasyMock.expect(request.getBucketName()).andReturn(bucket);
+ EasyMock.expectLastCall().anyTimes();
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r388686189
##########
File path: core/src/main/java/org/apache/druid/common/utils/TimeSupplier.java
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.druid.common.utils;
+
+import java.util.function.Supplier;
+
+public class TimeSupplier implements Supplier<Long>
Review comment:
I have mixed feelings about this class. It seems to exist in service of testing `S3TaskLogs.killAll`, but isn't that kind of leaking what is basically a test fixture, into the production code? Since the `killAll` method does nothing but delegate to the other call that takes an explicit timestamp, is this abstraction really worth having?
On the other hand, I can see an argument for using something like this to control system time, and I guess we already have similar situations sometimes when `@VisibleForTesting` are _only_ used by tests, so I'm not strictly against using this, just thinking out loud to have the discussion.
In the very least I think it should be renamed `CurrentTimeMillisSupplier` to indicate what time it is supplying, and add javadocs to describe its intended usage to ease testing.
Does this need to be setup in the module so that it gets injected into `S3TaskLogs`? (or did I miss that somewhere?)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r389185698
##########
File path: core/src/main/java/org/apache/druid/common/utils/TimeSupplier.java
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.druid.common.utils;
+
+import java.util.function.Supplier;
+
+public class TimeSupplier implements Supplier<Long>
Review comment:
This should be `LongSupplier` instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r388805794
##########
File path: extensions-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/S3TestUtils.java
##########
@@ -0,0 +1,212 @@
+/*
+ * 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.druid.storage.s3;
+
+import com.amazonaws.services.s3.model.DeleteObjectsRequest;
+import com.amazonaws.services.s3.model.ListObjectsV2Request;
+import com.amazonaws.services.s3.model.ListObjectsV2Result;
+import com.amazonaws.services.s3.model.S3ObjectSummary;
+import junit.framework.AssertionFailedError;
+import org.apache.commons.collections4.map.HashedMap;
+import org.easymock.EasyMock;
+import org.easymock.EasyMockSupport;
+import org.easymock.IArgumentMatcher;
+import org.easymock.IExpectationSetters;
+
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+public class S3TestUtils extends EasyMockSupport
+{
+ public static ListObjectsV2Request listObjectsV2RequestArgumentMatcher(ListObjectsV2Request listObjectsV2Request)
+ {
+ EasyMock.reportMatcher(new IArgumentMatcher()
+ {
+ @Override
+ public boolean matches(Object argument)
+ {
+
+ return argument instanceof ListObjectsV2Request
+ && listObjectsV2Request.getBucketName().equals(((ListObjectsV2Request) argument).getBucketName())
+ && listObjectsV2Request.getPrefix().equals(((ListObjectsV2Request) argument).getPrefix())
+ && ((listObjectsV2Request.getContinuationToken() == null
+ && ((ListObjectsV2Request) argument).getContinuationToken() == null)
+ || (listObjectsV2Request.getContinuationToken()
+ .equals(((ListObjectsV2Request) argument).getContinuationToken())))
+ && listObjectsV2Request.getMaxKeys().equals(((ListObjectsV2Request) argument).getMaxKeys());
+ }
+
+ @Override
+ public void appendTo(StringBuffer buffer)
+ {
+ String str = "ListObjectsV2Request(\"bucketName:\" \""
+ + listObjectsV2Request.getBucketName()
+ + "\", \"prefix:\""
+ + listObjectsV2Request.getPrefix()
+ + "\", \"continuationToken:\""
+ + listObjectsV2Request.getContinuationToken()
+ + "\", \"maxKeys:\""
+ + listObjectsV2Request.getMaxKeys()
+ + "\")";
+ buffer.append(str);
+ }
+ });
+ return null;
+ }
+
+ public static DeleteObjectsRequest deleteObjectsRequestArgumentMatcher(DeleteObjectsRequest deleteObjectsRequest)
+ {
+ EasyMock.reportMatcher(new IArgumentMatcher()
+ {
+ @Override
+ public boolean matches(Object argument)
+ {
+
+ boolean matches = argument instanceof DeleteObjectsRequest
+ && deleteObjectsRequest.getBucketName()
+ .equals(((DeleteObjectsRequest) argument).getBucketName())
+ && deleteObjectsRequest.getKeys().size() == ((DeleteObjectsRequest) argument).getKeys()
+ .size();
+ if (matches) {
+ Map<String, String> expectedKeysAndVersions = deleteObjectsRequest.getKeys().stream().collect(
+ Collectors.toMap(DeleteObjectsRequest.KeyVersion::getKey, x -> {
+ return x.getVersion() == null ? "null" : x.getVersion();
+ }));
+ Map<String, String> actualKeysAndVersions = ((DeleteObjectsRequest) argument).getKeys().stream().collect(
+ Collectors.toMap(DeleteObjectsRequest.KeyVersion::getKey, x -> {
+ return x.getVersion() == null ? "null" : x.getVersion();
+ }));
+ matches = expectedKeysAndVersions.equals(actualKeysAndVersions);
+ }
+ return matches;
+ }
+
+ @Override
+ public void appendTo(StringBuffer buffer)
+ {
+ String str = "DeleteObjectsRequest(\"bucketName:\" \""
+ + deleteObjectsRequest.getBucketName()
+ + "\", \"keys:\""
+ + deleteObjectsRequest.getKeys()
+ + "\")";
+ buffer.append(str);
+ }
+ });
+ return null;
+ }
+
+ public static S3ObjectSummary mockS3ObjectSummary(long lastModified, String key)
+ {
+ S3ObjectSummary objectSummary = EasyMock.createMock(S3ObjectSummary.class);
+ EasyMock.expect(objectSummary.getLastModified()).andReturn(new Date(lastModified));
+ EasyMock.expectLastCall().anyTimes();
+ EasyMock.expect(objectSummary.getKey()).andReturn(key);
+ EasyMock.expectLastCall().anyTimes();
+ return objectSummary;
+ }
+
+ public static ListObjectsV2Request mockRequest(
+ String bucket,
+ String prefix,
+ int maxKeys,
+ String continuationToken
+ )
+ {
+ ListObjectsV2Request request = EasyMock.createMock(ListObjectsV2Request.class);
+ EasyMock.expect(request.getBucketName()).andReturn(bucket);
+ EasyMock.expectLastCall().anyTimes();
Review comment:
Yes, you're right, noticing it in other places in the code. I will simplify, thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r390067077
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3Utils.java
##########
@@ -200,6 +204,54 @@ public static S3ObjectSummary getSingleObjectSummary(ServerSideEncryptingAmazonS
return objectSummary;
}
+ public static void deleteObjectsInPath(
Review comment:
nit: javadocs describing this method would be nice
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r388691614
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentKiller.java
##########
@@ -69,8 +85,48 @@ public void kill(DataSegment segment) throws SegmentLoadingException
}
@Override
- public void killAll()
+ public void killAll() throws IOException
{
- throw new UnsupportedOperationException("not implemented");
+ try {
+ S3Utils.retryS3Operation(
+ () -> {
+ String bucketName = segmentPusherConfig.getBucket();
+ String prefix = segmentPusherConfig.getBaseKey();
+ int maxListingLength = inputDataConfig.getMaxListingLength();
+ ListObjectsV2Result result;
+ String continuationToken = null;
+ do {
+ log.info("Deleting batch of %d segment files from s3 location [bucket: %s prefix: %s].",
Review comment:
`log.info` is a bit too informative for this operation I think, inside the loop at least. If you need to log anything, I suggest just counting the number of keys actually deleted and reporting the total at the end outside of the loop, or just a single message before the loop happens indicating that a some deletes are going to happen.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #9459:
Ability to Delete task logs and segments from S3
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #9459: Ability to Delete task logs and segments from S3
URL: https://github.com/apache/druid/pull/9459#discussion_r388808325
##########
File path: extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentKiller.java
##########
@@ -69,8 +85,48 @@ public void kill(DataSegment segment) throws SegmentLoadingException
}
@Override
- public void killAll()
+ public void killAll() throws IOException
{
- throw new UnsupportedOperationException("not implemented");
+ try {
+ S3Utils.retryS3Operation(
+ () -> {
+ String bucketName = segmentPusherConfig.getBucket();
+ String prefix = segmentPusherConfig.getBaseKey();
+ int maxListingLength = inputDataConfig.getMaxListingLength();
+ ListObjectsV2Result result;
+ String continuationToken = null;
+ do {
+ log.info("Deleting batch of %d segment files from s3 location [bucket: %s prefix: %s].",
Review comment:
Yeah I agree, I will log only once per invocation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org