suneet-s commented on code in PR #14131:
URL: https://github.com/apache/druid/pull/14131#discussion_r1181830073
##########
processing/src/main/java/org/apache/druid/segment/loading/DataSegmentKiller.java:
##########
@@ -54,6 +55,29 @@ static String descriptorPath(String path)
*/
void kill(DataSegment segment) throws SegmentLoadingException;
+ /**
+ * Kills a list of segments from deep storage. The default implementation
calls kill on the segments in a loop.
+ * Implementers of this interface can leverage batch / bulk deletes to be
more efficient. It is preferable to attempt
+ * to delete all segments even if there is an issue with deleting a single
one. This is up to implementers to
+ * implement as putting a try catch around the default kill via iteration
can be problematic if the client of the deep
+ * storage is unable to authenticate itself and segment loading exception
doesn't encode enough information in it to \
+ * understand why it failed.
+ * <p>
+ * If a segment or segments do not exist in deep storage method should not
throw an exception.
+ * <p>
+ * This version Kill must not require additional permissions on the deep
storage beyond what the single-arg
+ * form of kill @link org.apache.druid.segment.loading.DataSegmentKiller
requires.
Review Comment:
```suggestion
* This version of kill must **NOT** require additional permissions on the
deep storage beyond what
* {@link #kill(DataSegment)} requires.
```
##########
server/src/test/java/org/apache/druid/segment/loading/OmniDataSegmentKillerTest.java:
##########
@@ -119,6 +139,31 @@ public void testKillTombstone() throws Exception
segmentKiller.kill(tombstone);
}
+ @Test
+ public void testKillMultipleSegmentsWithType() throws SegmentLoadingException
+ {
+ final DataSegmentKiller killerSane = Mockito.mock(DataSegmentKiller.class);
+ final DataSegmentKiller killerSaneTwo =
Mockito.mock(DataSegmentKiller.class);
+ final DataSegment segment1 = Mockito.mock(DataSegment.class);
+ final DataSegment segment2 = Mockito.mock(DataSegment.class);
+ final DataSegment segment3 = Mockito.mock(DataSegment.class);
+ Mockito.when(segment1.isTombstone()).thenReturn(false);
+ Mockito.when(segment1.getLoadSpec()).thenReturn(ImmutableMap.of("type",
"sane"));
+ Mockito.when(segment2.isTombstone()).thenReturn(false);
+ Mockito.when(segment2.getLoadSpec()).thenReturn(ImmutableMap.of("type",
"sane"));
+ Mockito.when(segment3.isTombstone()).thenReturn(false);
+ Mockito.when(segment3.getLoadSpec()).thenReturn(ImmutableMap.of("type",
"sane_2"));
+
+ final Injector injector = createInjectorFromMap(ImmutableMap.of("sane",
killerSane, "sane_2", killerSaneTwo));
+ final OmniDataSegmentKiller segmentKiller =
injector.getInstance(OmniDataSegmentKiller.class);
+ segmentKiller.kill(ImmutableList.of(segment1, segment2, segment3));
+
+ Mockito.verify(killerSane, Mockito.times(1))
+ .kill((List<DataSegment>) argThat(containsInAnyOrder(segment1,
segment2)));
+ Mockito.verify(killerSaneTwo, Mockito.times(1))
+ .kill((List<DataSegment>) argThat(containsInAnyOrder(segment3)));
+ }
+
Review Comment:
Add tests for error handling please
##########
extensions-core/s3-extensions/src/test/java/org/apache/druid/storage/s3/S3DataSegmentKillerTest.java:
##########
@@ -213,4 +219,96 @@ public void
test_killAll_nonrecoverableExceptionWhenListingObjects_deletesAllSeg
Assert.assertTrue(ioExceptionThrown);
EasyMock.verify(s3Client, segmentPusherConfig, inputDataConfig);
}
+
+ @Test
+ public void test_kill_singleSegment_doesntexist_passes() throws
SegmentLoadingException
+ {
+ EasyMock.expect(s3Client.doesObjectExist(TEST_BUCKET,
KEY_1_PATH)).andReturn(false);
+ EasyMock.expectLastCall().once();
+ EasyMock.expect(s3Client.doesObjectExist(TEST_BUCKET,
KEY_1_DESCRIPTOR_PATH)).andReturn(false);
+ EasyMock.expectLastCall().once();
+ EasyMock.replay(s3Client, segmentPusherConfig, inputDataConfig);
+
+ segmentKiller = new S3DataSegmentKiller(Suppliers.ofInstance(s3Client),
segmentPusherConfig, inputDataConfig);
+ segmentKiller.kill(DATA_SEGMENT);
+ }
+
+ @Test
+ public void test_kill_singleSegment_exists_passes() throws
SegmentLoadingException
+ {
+ EasyMock.expect(s3Client.doesObjectExist(TEST_BUCKET,
KEY_1_PATH)).andReturn(true);
+ EasyMock.expectLastCall().once();
+
+ s3Client.deleteObject(TEST_BUCKET, KEY_1_PATH);
+ EasyMock.expectLastCall().andVoid();
+
+ EasyMock.expect(s3Client.doesObjectExist(TEST_BUCKET,
KEY_1_DESCRIPTOR_PATH)).andReturn(true);
+ EasyMock.expectLastCall().once();
+
+ s3Client.deleteObject(TEST_BUCKET, KEY_1_DESCRIPTOR_PATH);
+ EasyMock.expectLastCall().andVoid();
+
+ EasyMock.replay(s3Client, segmentPusherConfig, inputDataConfig);
+
+ segmentKiller = new S3DataSegmentKiller(Suppliers.ofInstance(s3Client),
segmentPusherConfig, inputDataConfig);
+ segmentKiller.kill(DATA_SEGMENT);
+ }
+
+ @Test
+ public void test_kill_listOfOneSegment() throws SegmentLoadingException
+ {
+ EasyMock.expect(s3Client.doesObjectExist(TEST_BUCKET,
KEY_1_PATH)).andReturn(true);
+ EasyMock.expectLastCall().once();
+
+ s3Client.deleteObject(TEST_BUCKET, KEY_1_PATH);
+ EasyMock.expectLastCall().andVoid();
+
+ EasyMock.expect(s3Client.doesObjectExist(TEST_BUCKET,
KEY_1_DESCRIPTOR_PATH)).andReturn(true);
+ EasyMock.expectLastCall().once();
+
+ s3Client.deleteObject(TEST_BUCKET, KEY_1_DESCRIPTOR_PATH);
+ EasyMock.expectLastCall().andVoid();
+
+
+ EasyMock.replay(s3Client, segmentPusherConfig, inputDataConfig);
+ segmentKiller = new S3DataSegmentKiller(Suppliers.ofInstance(s3Client),
segmentPusherConfig, inputDataConfig);
+ segmentKiller.kill(ImmutableList.of(DATA_SEGMENT));
+ }
+
+ @Test
+ public void test_kill_listOfNoSegments() throws SegmentLoadingException
+ {
+ EasyMock.replay(s3Client, segmentPusherConfig, inputDataConfig);
+ segmentKiller = new S3DataSegmentKiller(Suppliers.ofInstance(s3Client),
segmentPusherConfig, inputDataConfig);
+ segmentKiller.kill(ImmutableList.of());
+ // has an assertion error if there is an unexpected method call on a mock.
Do nothing because we expect the kill
+ // method to not interact with mocks
+ }
+
+ @Test
+ public void test_kill_listOfSegments() throws SegmentLoadingException
+ {
+ DeleteObjectsRequest deleteObjectsRequest = new
DeleteObjectsRequest(TEST_BUCKET);
+ deleteObjectsRequest.withKeys(KEY_1_PATH, KEY_1_PATH);
+ // struggled with the idea of making it match on equaling this
+ s3Client.deleteObjects(EasyMock.anyObject(DeleteObjectsRequest.class));
+ EasyMock.expectLastCall().andVoid().times(2);
+
+
+ EasyMock.replay(s3Client, segmentPusherConfig, inputDataConfig);
+ segmentKiller = new S3DataSegmentKiller(Suppliers.ofInstance(s3Client),
segmentPusherConfig, inputDataConfig);
+ segmentKiller.kill(ImmutableList.of(DATA_SEGMENT, DATA_SEGMENT));
+ }
+
+ private static final DataSegment DATA_SEGMENT = new DataSegment(
Review Comment:
Constants should be defined at the top of the file
--
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]
