kfaraz commented on code in PR #15673:
URL: https://github.com/apache/druid/pull/15673#discussion_r1451673119
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/http/OverlordResourceTest.java:
##########
@@ -983,10 +985,145 @@ public void testKillPendingSegments()
authConfig
);
- final Map<String, Integer> response = (Map<String, Integer>)
overlordResource
- .killPendingSegments("allow", new Interval(DateTimes.MIN,
DateTimes.nowUtc()).toString(), req)
- .getEntity();
- Assert.assertEquals(2, response.get("numDeleted").intValue());
+ Response resp = overlordResource
+ .killPendingSegments("allow", new Interval(DateTimes.MIN,
DateTimes.nowUtc()).toString(), req);
+
+ Assert.assertEquals(200, resp.getStatus());
+ Assert.assertEquals(ImmutableMap.of("numDeleted", 2), resp.getEntity());
+ }
+
+ @Test
+ public void testKillPendingSegmentsThrowsInvalidInputDruidException()
+ {
+ expectAuthorizationTokenCheck();
+
+ EasyMock.expect(taskMaster.isLeader()).andReturn(true);
+ final String exceptionMsg = "Some exception msg";
+ EasyMock
+ .expect(
+ indexerMetadataStorageAdapter.deletePendingSegments(
+ EasyMock.eq("allow"),
+ EasyMock.anyObject(Interval.class)
+ )
+ )
+ .andThrow(InvalidInput.exception(exceptionMsg))
+ .once();
+
+ EasyMock.replay(
+ taskRunner,
+ taskMaster,
+ taskStorageQueryAdapter,
+ indexerMetadataStorageAdapter,
+ req,
+ workerTaskRunnerQueryAdapter,
+ authConfig
+ );
+
+ Response resp = overlordResource
+ .killPendingSegments("allow", new Interval(DateTimes.MIN,
DateTimes.nowUtc()).toString(), req);
+
+ Assert.assertEquals(400, resp.getStatus());
+ Map<String, Object> respError = (Map) resp.getEntity();
+ Assert.assertTrue(respError.containsKey("error"));
+ Assert.assertEquals(exceptionMsg, respError.get("error"));
+ }
+
+ @Test
+ public void testKillPendingSegmentsThrowsDefensiveDruidException()
+ {
+ expectAuthorizationTokenCheck();
+
+ EasyMock.expect(taskMaster.isLeader()).andReturn(true);
+ final String exceptionMsg = "An internal defensive exception";
+ EasyMock
+ .expect(
+ indexerMetadataStorageAdapter.deletePendingSegments(
+ EasyMock.eq("allow"),
+ EasyMock.anyObject(Interval.class)
+ )
+ )
+ .andThrow(DruidException.defensive(exceptionMsg))
+ .once();
+
+ EasyMock.replay(
+ taskRunner,
+ taskMaster,
+ taskStorageQueryAdapter,
+ indexerMetadataStorageAdapter,
+ req,
+ workerTaskRunnerQueryAdapter,
+ authConfig
+ );
+
+ Response resp = overlordResource
+ .killPendingSegments("allow", new Interval(DateTimes.MIN,
DateTimes.nowUtc()).toString(), req);
+
+ Assert.assertEquals(500, resp.getStatus());
+ Map<String, Object> respError = (Map) resp.getEntity();
+ Assert.assertTrue(respError.containsKey("error"));
+ Assert.assertEquals(exceptionMsg, respError.get("error"));
+ }
+
+ @Test
+ public void testKillPendingSegmentsThrowsArbitraryException()
+ {
+ expectAuthorizationTokenCheck();
+
+ EasyMock.expect(taskMaster.isLeader()).andReturn(true);
+ final String exceptionMsg = "An unexpected illegal state exception";
+ EasyMock
+ .expect(
+ indexerMetadataStorageAdapter.deletePendingSegments(
+ EasyMock.eq("allow"),
+ EasyMock.anyObject(Interval.class)
+ )
+ )
+ .andThrow(new IllegalStateException(exceptionMsg))
+ .once();
+
+ EasyMock.replay(
+ taskRunner,
+ taskMaster,
+ taskStorageQueryAdapter,
+ indexerMetadataStorageAdapter,
+ req,
+ workerTaskRunnerQueryAdapter,
+ authConfig
+ );
+
+ Response resp = overlordResource
Review Comment:
Nit, here and other tests: `response` and `responseEntity` seem like more
appropriate variable names than `resp` and `respError`.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/IndexerMetadataStorageAdapter.java:
##########
@@ -46,23 +47,31 @@ public IndexerMetadataStorageAdapter(
public int deletePendingSegments(String dataSource, Interval deleteInterval)
{
- // Check the given interval overlaps the
interval(minCreatedDateOfActiveTasks, MAX)
- final Optional<DateTime> minCreatedDateOfActiveTasks =
taskStorageQueryAdapter
+ // Find the earliest active task created for the specified datasource; if
one exists,
+ // check if its interval overlaps with the delete interval.
+ final Optional<TaskInfo<Task, TaskStatus>> earliestActiveTaskStatusInfo =
taskStorageQueryAdapter
.getActiveTaskInfo(dataSource)
.stream()
- .map(TaskInfo::getCreatedTime)
- .min(Comparator.naturalOrder());
+ .min(Comparator.comparing(TaskInfo::getCreatedTime));
- final Interval activeTaskInterval = new Interval(
- minCreatedDateOfActiveTasks.orElse(DateTimes.MAX),
- DateTimes.MAX
- );
+ if (earliestActiveTaskStatusInfo.isPresent()) {
+ final TaskInfo<Task, TaskStatus> theEarliestActiveTaskInfo =
earliestActiveTaskStatusInfo.get();
Review Comment:
Nit:
Rather than using names like `earliestActiveInfo` and
`theEarliestActiveInfo`, you might as well just call the first one,
`optionalEarliestActiveInfo`.
Personal opinion:
You could even try simpler names without losing out on any useful
information.
e.g. `earliestActiveTaskOptional` and `earliestActiveTask` (the suffix
`StatusInfo` doesn't seem very useful here).
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/IndexerMetadataStorageAdapterTest.java:
##########
@@ -20,28 +20,25 @@
package org.apache.druid.indexing.overlord;
import com.google.common.collect.ImmutableList;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.error.DruidExceptionMatcher;
import org.apache.druid.indexer.TaskInfo;
import org.apache.druid.indexer.TaskStatus;
import org.apache.druid.indexing.common.task.NoopTask;
import org.apache.druid.indexing.common.task.Task;
import org.apache.druid.java.util.common.DateTimes;
import org.apache.druid.java.util.common.Intervals;
import org.easymock.EasyMock;
-import org.hamcrest.CoreMatchers;
+import org.hamcrest.MatcherAssert;
import org.joda.time.Interval;
import org.junit.Assert;
import org.junit.Before;
-import org.junit.Rule;
import org.junit.Test;
-import org.junit.rules.ExpectedException;
import java.util.List;
public class IndexerMetadataStorageAdapterTest
{
- @Rule
- public ExpectedException expectedException = ExpectedException.none();
Review Comment:
Thanks for removing this!
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/IndexerMetadataStorageAdapter.java:
##########
@@ -46,23 +47,31 @@ public IndexerMetadataStorageAdapter(
public int deletePendingSegments(String dataSource, Interval deleteInterval)
{
- // Check the given interval overlaps the
interval(minCreatedDateOfActiveTasks, MAX)
- final Optional<DateTime> minCreatedDateOfActiveTasks =
taskStorageQueryAdapter
+ // Find the earliest active task created for the specified datasource; if
one exists,
+ // check if its interval overlaps with the delete interval.
+ final Optional<TaskInfo<Task, TaskStatus>> earliestActiveTaskStatusInfo =
taskStorageQueryAdapter
.getActiveTaskInfo(dataSource)
.stream()
- .map(TaskInfo::getCreatedTime)
- .min(Comparator.naturalOrder());
+ .min(Comparator.comparing(TaskInfo::getCreatedTime));
- final Interval activeTaskInterval = new Interval(
- minCreatedDateOfActiveTasks.orElse(DateTimes.MAX),
- DateTimes.MAX
- );
+ if (earliestActiveTaskStatusInfo.isPresent()) {
+ final TaskInfo<Task, TaskStatus> theEarliestActiveTaskInfo =
earliestActiveTaskStatusInfo.get();
+ final Interval activeTaskInterval = new Interval(
+ theEarliestActiveTaskInfo.getCreatedTime(),
+ DateTimes.MAX
+ );
- Preconditions.checkArgument(
- !deleteInterval.overlaps(activeTaskInterval),
- "Cannot delete pendingSegments because there is at least one active
task created at %s",
- activeTaskInterval.getStart()
- );
+ if (deleteInterval.overlaps(activeTaskInterval)) {
+ throw InvalidInput.exception(
+ "Cannot delete pendingSegments for datasource[%s] as there's at
least one active task[%s] created at[%s] "
Review Comment:
```suggestion
"Cannot delete pendingSegments for datasource[%s] as there is an
active task[%s] created at[%s] "
```
--
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]
