kfaraz commented on code in PR #19009:
URL: https://github.com/apache/druid/pull/19009#discussion_r2820339531
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentKiller.java:
##########
@@ -212,6 +208,24 @@ private boolean deleteKeysForBucket(
return hadException;
}
+ private boolean deleteSegmentFilesFromS3(String s3Bucket, String s3Path)
+ {
+ try {
+ S3Utils.deleteObjectsInPath(
+ s3ClientSupplier.get(),
+ inputDataConfig.getMaxListingLength(),
+ s3Bucket,
+ s3Path,
+ Predicates.alwaysTrue()
+ );
+ }
+ catch (Exception e) {
+ log.error("Error occurred while deleting segment files from s3. Error:
%s", e.getMessage());
Review Comment:
This would log the stack trace as well.
```suggestion
log.error(e, "Error occurred while deleting segment files from s3.");
```
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentKiller.java:
##########
@@ -223,12 +237,17 @@ public void kill(DataSegment segment) throws
SegmentLoadingException
if (s3Path.endsWith("/")) {
// segment is not compressed, list objects and delete them all
- final ListObjectsV2Result list = s3Client.listObjectsV2(
- new
ListObjectsV2Request().withBucketName(s3Bucket).withPrefix(s3Path)
- );
- for (S3ObjectSummary objectSummary : list.getObjectSummaries()) {
- log.info("Removing index file[s3://%s/%s] from s3!", s3Bucket,
objectSummary.getKey());
- s3Client.deleteObject(s3Bucket, objectSummary.getKey());
+ try {
Review Comment:
Nit: Maybe just call the new method `deleteSegmentFilesFromS3` here, since
the logic seems to be the same.
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentKiller.java:
##########
@@ -104,19 +103,16 @@ public void kill(List<DataSegment> segments) throws
SegmentLoadingException
);
if (path.endsWith("/")) {
// segment is not compressed, list objects and add them all to delete
list
- final ListObjectsV2Result list = s3Client.listObjectsV2(
- new
ListObjectsV2Request().withBucketName(s3Bucket).withPrefix(path)
- );
- for (S3ObjectSummary objectSummary : list.getObjectSummaries()) {
- keysToDelete.add(new
DeleteObjectsRequest.KeyVersion(objectSummary.getKey()));
+ boolean hadException = deleteSegmentFilesFromS3(s3Bucket, path);
+ if (hadException) {
Review Comment:
```suggestion
boolean success = deleteSegmentFilesFromS3(s3Bucket, path);
if (!success) {
```
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentKiller.java:
##########
@@ -223,12 +237,17 @@ public void kill(DataSegment segment) throws
SegmentLoadingException
if (s3Path.endsWith("/")) {
// segment is not compressed, list objects and delete them all
- final ListObjectsV2Result list = s3Client.listObjectsV2(
- new
ListObjectsV2Request().withBucketName(s3Bucket).withPrefix(s3Path)
- );
- for (S3ObjectSummary objectSummary : list.getObjectSummaries()) {
- log.info("Removing index file[s3://%s/%s] from s3!", s3Bucket,
objectSummary.getKey());
- s3Client.deleteObject(s3Bucket, objectSummary.getKey());
+ try {
+ S3Utils.deleteObjectsInPath(
+ s3ClientSupplier.get(),
+ inputDataConfig.getMaxListingLength(),
+ s3Bucket,
+ s3Path,
+ Predicates.alwaysTrue()
+ );
+ }
+ catch (Exception e) {
+ log.error("Error occurred while deleting segment files from s3.
Error: %s", e.getMessage());
Review Comment:
```suggestion
log.error(e, "Error occurred while deleting segment files from
s3.");
```
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentKiller.java:
##########
@@ -212,6 +208,24 @@ private boolean deleteKeysForBucket(
return hadException;
}
+ private boolean deleteSegmentFilesFromS3(String s3Bucket, String s3Path)
+ {
+ try {
+ S3Utils.deleteObjectsInPath(
+ s3ClientSupplier.get(),
+ inputDataConfig.getMaxListingLength(),
+ s3Bucket,
+ s3Path,
+ Predicates.alwaysTrue()
+ );
+ }
+ catch (Exception e) {
+ log.error("Error occurred while deleting segment files from s3. Error:
%s", e.getMessage());
+ return true;
Review Comment:
Please invert these return values as it is more natural to return `true` for
success scenarios and `false` for failures.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
