gianm commented on code in PR #18891:
URL: https://github.com/apache/druid/pull/18891#discussion_r2912914607
##########
extensions-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/S3DataSegmentKillerTest.java:
##########
@@ -383,22 +392,26 @@ public void
test_kill_listOfSegments_multiDeleteExceptionIsThrown()
@Test
public void
test_kill_listOfSegments_multiDeleteExceptionIsThrownMultipleTimes()
{
- DeleteObjectsRequest deleteObjectsRequest = new
DeleteObjectsRequest(TEST_BUCKET);
Review Comment:
Same issue as `test_kill_listOfSegments_multiDeleteExceptionIsThrown`. We
should use a regular `DeleteObjectsResponse` with errors set.
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3DataSegmentKiller.java:
##########
@@ -147,65 +152,69 @@ public void kill(List<DataSegment> segments) throws
SegmentLoadingException
private boolean deleteKeysForBucket(
ServerSideEncryptingAmazonS3 s3Client,
String s3Bucket,
- List<DeleteObjectsRequest.KeyVersion> keysToDelete
+ List<ObjectIdentifier> keysToDelete
)
{
boolean hadException = false;
- DeleteObjectsRequest deleteObjectsRequest = new
DeleteObjectsRequest(s3Bucket);
- deleteObjectsRequest.setQuiet(true);
- List<List<DeleteObjectsRequest.KeyVersion>> keysChunks = Lists.partition(
+ List<List<ObjectIdentifier>> keysChunks = Lists.partition(
keysToDelete,
MAX_MULTI_OBJECT_DELETE_SIZE
);
- for (List<DeleteObjectsRequest.KeyVersion> chunkOfKeys : keysChunks) {
+ for (List<ObjectIdentifier> chunkOfKeys : keysChunks) {
List<String> keysToDeleteStrings = chunkOfKeys.stream().map(
-
DeleteObjectsRequest.KeyVersion::getKey).collect(Collectors.toList());
+ ObjectIdentifier::key).collect(Collectors.toList());
try {
- deleteObjectsRequest.setKeys(chunkOfKeys);
+ DeleteObjectsRequest deleteObjectsRequest =
DeleteObjectsRequest.builder()
+ .bucket(s3Bucket)
+ .delete(Delete.builder()
+ .objects(chunkOfKeys)
+ .quiet(true)
+ .build())
+ .build();
log.info(
"Deleting the following segment files from S3 bucket[%s]: [%s]",
s3Bucket,
keysToDeleteStrings
);
S3Utils.retryS3Operation(
() -> {
- s3Client.deleteObjects(deleteObjectsRequest);
+ DeleteObjectsResponse response =
s3Client.deleteObjects(deleteObjectsRequest);
+ // Check for errors in the response
+ if (response.hasErrors()) {
Review Comment:
The old code for `deleteKeysForBucket`, if there was an error in a
multi-delete, would set `hadException = true` and then return `true` (meaning
there was an issue deleting keys). That would then cause the caller (`kill`) to
throw a `SegmentLoadingException`. Now, the exception is suppressed and `kill`
will treat it as a success. Please restore the old behavior.
There's a retry issue too. The old code would sometimes retry
`MultiObjectDeleteException` (see my comment in `S3Utils`) due to some logic
for retrying it in `retryS3Operation`. The new code does not retry, just
silently treats it as a success (after logging some warnings).
It may be best to focus on fixing `S3Utils.deleteBucketKeys` and then use
that here. Then we only need to get the error/retry logic right in one place.
##########
extensions-core/s3-extensions/src/main/java/org/apache/druid/storage/s3/S3Utils.java:
##########
@@ -328,18 +418,21 @@ public static void deleteObjectsInPath(
public static void deleteBucketKeys(
ServerSideEncryptingAmazonS3 s3Client,
String bucket,
- List<DeleteObjectsRequest.KeyVersion> keysToDelete,
+ List<ObjectIdentifier> keysToDelete,
int retries
)
throws Exception
{
if (keysToDelete != null && log.isDebugEnabled()) {
List<String> keys = keysToDelete.stream()
-
.map(DeleteObjectsRequest.KeyVersion::getKey)
+ .map(ObjectIdentifier::key)
.collect(Collectors.toList());
log.debug("Deleting keys from bucket: [%s], keys: [%s]", bucket, keys);
}
- DeleteObjectsRequest deleteRequest = new
DeleteObjectsRequest(bucket).withKeys(keysToDelete);
+ DeleteObjectsRequest deleteRequest = DeleteObjectsRequest.builder()
+ .bucket(bucket)
+ .delete(Delete.builder().objects(keysToDelete).build())
+ .build();
S3Utils.retryS3Operation(() -> {
s3Client.deleteObjects(deleteRequest);
Review Comment:
Need to check `hasErrors` in the response. Related: the old `S3RETRY`
checked for `MultiObjectDeleteException` and would retry in certain cases.
That's lost now, please restore it. The best way I can think of to do that is
to introduce our own version of `MultiObjectDeleteException` and throw it when
`hasErrors` is true, and then add our version to `S3RETRY`.
Please add a test for this scenario too.
For extra credit only retry the objects that failed to be deleted!
##########
extensions-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/S3DataSegmentKillerTest.java:
##########
@@ -359,16 +365,19 @@ public void test_kill_listOfSegments() throws
SegmentLoadingException
@Test
public void test_kill_listOfSegments_multiDeleteExceptionIsThrown()
{
- DeleteObjectsRequest deleteObjectsRequest = new
DeleteObjectsRequest(TEST_BUCKET);
- deleteObjectsRequest.withKeys(KEY_1_PATH, KEY_2_PATH);
// struggled with the idea of making it match on equaling this
s3Client.deleteObjects(EasyMock.anyObject(DeleteObjectsRequest.class));
- MultiObjectDeleteException.DeleteError deleteError = new
MultiObjectDeleteException.DeleteError();
- deleteError.setKey(KEY_1_PATH);
- MultiObjectDeleteException multiObjectDeleteException = new
MultiObjectDeleteException(
- ImmutableList.of(deleteError),
- ImmutableList.of());
- EasyMock.expectLastCall().andThrow(multiObjectDeleteException).once();
+ // In v2 SDK, multi-object delete errors are returned in the response, not
thrown as exceptions
+ // For testing purposes, we simulate an S3Exception being thrown
+ S3Exception s3Exception = (S3Exception) S3Exception.builder()
Review Comment:
This isn't right, we should test what the v2 SDK actually does. The test
should use a regular `DeleteObjectsResponse` with errors set.
##########
extensions-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/S3DataSegmentKillerTest.java:
##########
@@ -438,20 +453,20 @@ public void
test_kill_listOfSegments_amazonServiceExceptionExceptionIsThrown()
@Test
public void test_kill_listOfSegments_retryableExceptionThrown() throws
SegmentLoadingException
Review Comment:
This test was formerly testing specifically the partial failure case. Please
add back a test that focuses on this case. In v2 it will show up as `hasErrors`
on the response.
--
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]
