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 2021/07/27 07:45:33 UTC

[GitHub] [druid] jon-wei opened a new pull request #11501: Allow kill task to mark segments as unused

jon-wei opened a new pull request #11501:
URL: https://github.com/apache/druid/pull/11501


   This PR adds a new `markAsUnused` option for the Kill Task, which allows it to mark any segments within the specified interval as unused before deleting the unused segments within an interval.
   
   This is useful for allowing the mark unused -> delete sequence to happen with a single API call for the caller, as well as allowing the unmark action to occur under a task interval lock.
   
   This PR has:
   - [x] been self-reviewed.
   - [x] 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/dev/license.md)
   - [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
   - [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
   - [ ] added integration tests.
   - [x] been tested in a test Druid cluster.
   


-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org


[GitHub] [druid] jon-wei merged pull request #11501: Allow kill task to mark segments as unused

Posted by GitBox <gi...@apache.org>.
jon-wei merged pull request #11501:
URL: https://github.com/apache/druid/pull/11501


   


-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org


[GitHub] [druid] jon-wei commented on a change in pull request #11501: Allow kill task to mark segments as unused

Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #11501:
URL: https://github.com/apache/druid/pull/11501#discussion_r677800795



##########
File path: server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
##########
@@ -226,6 +226,33 @@ public void start()
     return matchingSegments;
   }
 
+  @Override
+  public int markSegmentsAsUnusedWithinInterval(String dataSource, Interval interval)
+  {
+    int numSegmentsMarkedUnused = connector.retryTransaction(
+        (handle, status) -> {
+          return handle
+              .createStatement(
+                  StringUtils.format(
+                      "UPDATE %s SET used=false WHERE dataSource = :dataSource "
+                      + "AND start >= :start AND %2$send%2$s <= :end",

Review comment:
       Hm, I used the same query as run by the `markUnused` endpoint on the coordinator:
   https://github.com/apache/druid/blob/master/server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java#L836
   
   




-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
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 #11501: Allow kill task to mark segments as unused

Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #11501:
URL: https://github.com/apache/druid/pull/11501#discussion_r677867443



##########
File path: indexing-service/src/test/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTaskTest.java
##########
@@ -74,7 +74,58 @@ public void testKill() throws Exception
     );
 
     final KillUnusedSegmentsTask task =
-        new KillUnusedSegmentsTask(null, DATA_SOURCE, Intervals.of("2019-03-01/2019-04-01"), null);
+        new KillUnusedSegmentsTask(
+            null,
+            DATA_SOURCE,
+            Intervals.of("2019-03-01/2019-04-01"),
+            null,
+            false
+        );
+
+    Assert.assertEquals(TaskState.SUCCESS, taskRunner.run(task).get().getStatusCode());
+
+    final List<DataSegment> unusedSegments =
+        getMetadataStorageCoordinator().retrieveUnusedSegmentsForInterval(DATA_SOURCE, Intervals.of("2019/2020"));
+
+    Assert.assertEquals(ImmutableList.of(newSegment(Intervals.of("2019-02-01/2019-03-01"), version)), unusedSegments);
+    Assertions.assertThat(
+        getMetadataStorageCoordinator()
+            .retrieveUsedSegmentsForInterval(DATA_SOURCE, Intervals.of("2019/2020"), Segments.ONLY_VISIBLE)
+    ).containsExactlyInAnyOrder(
+        newSegment(Intervals.of("2019-01-01/2019-02-01"), version),
+        newSegment(Intervals.of("2019-04-01/2019-05-01"), version)
+    );
+  }
+
+
+  @Test
+  public void testKillWithMarkUnused() throws Exception
+  {
+    final String version = DateTimes.nowUtc().toString();
+    final Set<DataSegment> segments = ImmutableSet.of(
+        newSegment(Intervals.of("2019-01-01/2019-02-01"), version),
+        newSegment(Intervals.of("2019-02-01/2019-03-01"), version),
+        newSegment(Intervals.of("2019-03-01/2019-04-01"), version),
+        newSegment(Intervals.of("2019-04-01/2019-05-01"), version)
+    );
+    final Set<DataSegment> announced = getMetadataStorageCoordinator().announceHistoricalSegments(segments);
+
+    Assert.assertEquals(segments, announced);
+
+    Assert.assertTrue(
+        getSegmentsMetadataManager().markSegmentAsUnused(
+            newSegment(Intervals.of("2019-02-01/2019-03-01"), version).getId().toString()
+        )
+    );
+
+    final KillUnusedSegmentsTask task =
+        new KillUnusedSegmentsTask(
+            null,
+            DATA_SOURCE,
+            Intervals.of("2019-03-01/2019-04-01"),
+            null,
+            true
+        );

Review comment:
       The conditions that the test here is checking seem to be the same for the case that MarkUnused is false. Any other condition(s) here that we can check here to differentiate the 2 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.

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org


[GitHub] [druid] jon-wei commented on a change in pull request #11501: Allow kill task to mark segments as unused

Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #11501:
URL: https://github.com/apache/druid/pull/11501#discussion_r677903038



##########
File path: server/src/main/java/org/apache/druid/client/indexing/HttpIndexingServiceClient.java
##########
@@ -70,7 +70,7 @@ public HttpIndexingServiceClient(
   public void killUnusedSegments(String idPrefix, String dataSource, Interval interval)

Review comment:
       The two places where it's used currently:
   
   ```
     @DELETE
     @Path("/{dataSourceName}/intervals/{interval}")
   ```
   on the coordinator which just issues a kill task
   
   and internally by the coordinator in a coordinator duty which periodically issues a kill task for segments that are unused (if that functionality is enabled in the coordinator).
   
   For the former case, it might make sense to add this parameter, but the same API set already has the "markUnused" API, so I didn't feel it was necessary and wanted to limit the scope of the PR, and for the latter case, the "markUnused" flag doesn't make sense to enable.




-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
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 #11501: Allow kill task to mark segments as unused

Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11501:
URL: https://github.com/apache/druid/pull/11501#discussion_r677814510



##########
File path: server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
##########
@@ -226,6 +226,33 @@ public void start()
     return matchingSegments;
   }
 
+  @Override
+  public int markSegmentsAsUnusedWithinInterval(String dataSource, Interval interval)
+  {
+    int numSegmentsMarkedUnused = connector.retryTransaction(
+        (handle, status) -> {
+          return handle
+              .createStatement(
+                  StringUtils.format(
+                      "UPDATE %s SET used=false WHERE dataSource = :dataSource "
+                      + "AND start >= :start AND %2$send%2$s <= :end",

Review comment:
       Sorry NVM. I was confused.




-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org


[GitHub] [druid] jon-wei commented on a change in pull request #11501: Allow kill task to mark segments as unused

Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #11501:
URL: https://github.com/apache/druid/pull/11501#discussion_r677838673



##########
File path: docs/ingestion/data-management.md
##########
@@ -95,18 +95,23 @@ A data deletion tutorial is available at [Tutorial: Deleting data](../tutorials/
 
 ## Kill Task
 
-Kill tasks delete all information about a segment and removes it from deep storage. Segments to kill must be unused (used==0) in the Druid segment table. The available grammar is:
+Kill tasks delete all information about a segment and removes it from deep storage. Segments to kill must be unused (used==0) in the Druid segment table.
+
+The available grammar is:
 
 ```json
 {
     "type": "kill",
     "id": <task_id>,
     "dataSource": <task_datasource>,
     "interval" : <all_segments_in_this_interval_will_die!>,
+    "markAsUnused": <true|false>,
     "context": <task context>
 }
 ```
 
+If `markAsUnused` is true, the kill task will first mark any segments within the specified interval as unused, before deleting the unused segments within the interval.

Review comment:
       I added a WARNING section to the kill task




-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
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 #11501: Allow kill task to mark segments as unused

Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11501:
URL: https://github.com/apache/druid/pull/11501#discussion_r677779492



##########
File path: docs/ingestion/data-management.md
##########
@@ -95,18 +95,23 @@ A data deletion tutorial is available at [Tutorial: Deleting data](../tutorials/
 
 ## Kill Task
 
-Kill tasks delete all information about a segment and removes it from deep storage. Segments to kill must be unused (used==0) in the Druid segment table. The available grammar is:
+Kill tasks delete all information about a segment and removes it from deep storage. Segments to kill must be unused (used==0) in the Druid segment table.

Review comment:
       Hmm maybe
    
   ```suggestion
   The kill task deletes all information about segments and removes them from deep storage. Segments to kill must be unused (used==0) in the Druid segment table.
   ```

##########
File path: docs/ingestion/data-management.md
##########
@@ -95,18 +95,23 @@ A data deletion tutorial is available at [Tutorial: Deleting data](../tutorials/
 
 ## Kill Task
 
-Kill tasks delete all information about a segment and removes it from deep storage. Segments to kill must be unused (used==0) in the Druid segment table. The available grammar is:
+Kill tasks delete all information about a segment and removes it from deep storage. Segments to kill must be unused (used==0) in the Druid segment table.
+
+The available grammar is:
 
 ```json
 {
     "type": "kill",
     "id": <task_id>,
     "dataSource": <task_datasource>,
     "interval" : <all_segments_in_this_interval_will_die!>,
+    "markAsUnused": <true|false>,

Review comment:
       It seems worth to mention what the default is.

##########
File path: server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java
##########
@@ -226,6 +226,33 @@ public void start()
     return matchingSegments;
   }
 
+  @Override
+  public int markSegmentsAsUnusedWithinInterval(String dataSource, Interval interval)
+  {
+    int numSegmentsMarkedUnused = connector.retryTransaction(
+        (handle, status) -> {
+          return handle
+              .createStatement(
+                  StringUtils.format(
+                      "UPDATE %s SET used=false WHERE dataSource = :dataSource "
+                      + "AND start >= :start AND %2$send%2$s <= :end",

Review comment:
       Hmm should the end be exclusive? I see `IndexerSQLMetadataStorageCoordinator.retrieveUnusedSegmentsForInterval()` uses the same filter of `"end" <= :end`, which seems to break the contract of `IndexerMetadataStorageCoordinator.retrieveUnusedSegmentsForInterval()` because the end time is exclusive for segment intervals. Maybe this hasn't caused much troubles so far because `retrieveUnusedSegmentsForInterval` returns only unused segments, even though it seems like a bug. But this method will unset the used flag and thus probably should respect the exclusivity of the end time?

##########
File path: docs/ingestion/data-management.md
##########
@@ -95,18 +95,23 @@ A data deletion tutorial is available at [Tutorial: Deleting data](../tutorials/
 
 ## Kill Task
 
-Kill tasks delete all information about a segment and removes it from deep storage. Segments to kill must be unused (used==0) in the Druid segment table. The available grammar is:
+Kill tasks delete all information about a segment and removes it from deep storage. Segments to kill must be unused (used==0) in the Druid segment table.
+
+The available grammar is:
 
 ```json
 {
     "type": "kill",
     "id": <task_id>,
     "dataSource": <task_datasource>,
     "interval" : <all_segments_in_this_interval_will_die!>,
+    "markAsUnused": <true|false>,
     "context": <task context>
 }
 ```
 
+If `markAsUnused` is true, the kill task will first mark any segments within the specified interval as unused, before deleting the unused segments within the interval.

Review comment:
       Perhaps we could make this more scary because it cannot be undone once they delete segments with `markAsUnused` set?




-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org


[GitHub] [druid] jon-wei commented on a change in pull request #11501: Allow kill task to mark segments as unused

Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #11501:
URL: https://github.com/apache/druid/pull/11501#discussion_r677899038



##########
File path: indexing-service/src/test/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTaskTest.java
##########
@@ -74,7 +74,58 @@ public void testKill() throws Exception
     );
 
     final KillUnusedSegmentsTask task =
-        new KillUnusedSegmentsTask(null, DATA_SOURCE, Intervals.of("2019-03-01/2019-04-01"), null);
+        new KillUnusedSegmentsTask(
+            null,
+            DATA_SOURCE,
+            Intervals.of("2019-03-01/2019-04-01"),
+            null,
+            false
+        );
+
+    Assert.assertEquals(TaskState.SUCCESS, taskRunner.run(task).get().getStatusCode());
+
+    final List<DataSegment> unusedSegments =
+        getMetadataStorageCoordinator().retrieveUnusedSegmentsForInterval(DATA_SOURCE, Intervals.of("2019/2020"));
+
+    Assert.assertEquals(ImmutableList.of(newSegment(Intervals.of("2019-02-01/2019-03-01"), version)), unusedSegments);
+    Assertions.assertThat(
+        getMetadataStorageCoordinator()
+            .retrieveUsedSegmentsForInterval(DATA_SOURCE, Intervals.of("2019/2020"), Segments.ONLY_VISIBLE)
+    ).containsExactlyInAnyOrder(
+        newSegment(Intervals.of("2019-01-01/2019-02-01"), version),
+        newSegment(Intervals.of("2019-04-01/2019-05-01"), version)
+    );
+  }
+
+
+  @Test
+  public void testKillWithMarkUnused() throws Exception
+  {
+    final String version = DateTimes.nowUtc().toString();
+    final Set<DataSegment> segments = ImmutableSet.of(
+        newSegment(Intervals.of("2019-01-01/2019-02-01"), version),
+        newSegment(Intervals.of("2019-02-01/2019-03-01"), version),
+        newSegment(Intervals.of("2019-03-01/2019-04-01"), version),
+        newSegment(Intervals.of("2019-04-01/2019-05-01"), version)
+    );
+    final Set<DataSegment> announced = getMetadataStorageCoordinator().announceHistoricalSegments(segments);
+
+    Assert.assertEquals(segments, announced);
+
+    Assert.assertTrue(
+        getSegmentsMetadataManager().markSegmentAsUnused(
+            newSegment(Intervals.of("2019-02-01/2019-03-01"), version).getId().toString()
+        )
+    );
+
+    final KillUnusedSegmentsTask task =
+        new KillUnusedSegmentsTask(
+            null,
+            DATA_SOURCE,
+            Intervals.of("2019-03-01/2019-04-01"),
+            null,
+            true
+        );

Review comment:
       The intent was to have it be a very similar test, in the MarkUnused=false test, there is a manual "mark unused" call:
   
   ```
   Assert.assertTrue(
           getSegmentsMetadataManager().markSegmentAsUnused(
               newSegment(Intervals.of("2019-03-01/2019-04-01"), version).getId().toString()
           )
       );
   ```
   
   which is now replaced by "markUnused=true" + providing that interval ("2019-03-01/2019-04-01").




-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org


[GitHub] [druid] zachjsh commented on pull request #11501: Allow kill task to mark segments as unused

Posted by GitBox <gi...@apache.org>.
zachjsh commented on pull request #11501:
URL: https://github.com/apache/druid/pull/11501#issuecomment-887904418


   Looks good, just had a few comments. Also is it doable to add integration test for this? Can we piggy back on existiing integration test?


-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org


[GitHub] [druid] jon-wei commented on a change in pull request #11501: Allow kill task to mark segments as unused

Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #11501:
URL: https://github.com/apache/druid/pull/11501#discussion_r677903038



##########
File path: server/src/main/java/org/apache/druid/client/indexing/HttpIndexingServiceClient.java
##########
@@ -70,7 +70,7 @@ public HttpIndexingServiceClient(
   public void killUnusedSegments(String idPrefix, String dataSource, Interval interval)

Review comment:
       The two places where it's used currently:
   
   ```
     @DELETE
     @Path("/{dataSourceName}/intervals/{interval}")
   ```
   on the coordinator which just issues a kill task
   
   and internally by the coordinator in a coordinator duty which periodically issues a kill task for segments that are unused (if that functionality is enabled in the coordinator).
   
   For the former case, it might make sense to add this parameter, but the same API set already has the "markUnused" API, so I didn't feel it was necessary and wanted to limit the scope of the PR, and for the latter case, the "markUnused" flag doesn't make sense.




-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org


[GitHub] [druid] jon-wei commented on a change in pull request #11501: Allow kill task to mark segments as unused

Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #11501:
URL: https://github.com/apache/druid/pull/11501#discussion_r677899038



##########
File path: indexing-service/src/test/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTaskTest.java
##########
@@ -74,7 +74,58 @@ public void testKill() throws Exception
     );
 
     final KillUnusedSegmentsTask task =
-        new KillUnusedSegmentsTask(null, DATA_SOURCE, Intervals.of("2019-03-01/2019-04-01"), null);
+        new KillUnusedSegmentsTask(
+            null,
+            DATA_SOURCE,
+            Intervals.of("2019-03-01/2019-04-01"),
+            null,
+            false
+        );
+
+    Assert.assertEquals(TaskState.SUCCESS, taskRunner.run(task).get().getStatusCode());
+
+    final List<DataSegment> unusedSegments =
+        getMetadataStorageCoordinator().retrieveUnusedSegmentsForInterval(DATA_SOURCE, Intervals.of("2019/2020"));
+
+    Assert.assertEquals(ImmutableList.of(newSegment(Intervals.of("2019-02-01/2019-03-01"), version)), unusedSegments);
+    Assertions.assertThat(
+        getMetadataStorageCoordinator()
+            .retrieveUsedSegmentsForInterval(DATA_SOURCE, Intervals.of("2019/2020"), Segments.ONLY_VISIBLE)
+    ).containsExactlyInAnyOrder(
+        newSegment(Intervals.of("2019-01-01/2019-02-01"), version),
+        newSegment(Intervals.of("2019-04-01/2019-05-01"), version)
+    );
+  }
+
+
+  @Test
+  public void testKillWithMarkUnused() throws Exception
+  {
+    final String version = DateTimes.nowUtc().toString();
+    final Set<DataSegment> segments = ImmutableSet.of(
+        newSegment(Intervals.of("2019-01-01/2019-02-01"), version),
+        newSegment(Intervals.of("2019-02-01/2019-03-01"), version),
+        newSegment(Intervals.of("2019-03-01/2019-04-01"), version),
+        newSegment(Intervals.of("2019-04-01/2019-05-01"), version)
+    );
+    final Set<DataSegment> announced = getMetadataStorageCoordinator().announceHistoricalSegments(segments);
+
+    Assert.assertEquals(segments, announced);
+
+    Assert.assertTrue(
+        getSegmentsMetadataManager().markSegmentAsUnused(
+            newSegment(Intervals.of("2019-02-01/2019-03-01"), version).getId().toString()
+        )
+    );
+
+    final KillUnusedSegmentsTask task =
+        new KillUnusedSegmentsTask(
+            null,
+            DATA_SOURCE,
+            Intervals.of("2019-03-01/2019-04-01"),
+            null,
+            true
+        );

Review comment:
       The intent was to have it be a very similar test, in the MarkUnused=false test, there is a manual "mark unused" call:
   
   ```
   Assert.assertTrue(
           getSegmentsMetadataManager().markSegmentAsUnused(
               newSegment(Intervals.of("2019-03-01/2019-04-01"), version).getId().toString()
           )
       );
   ```
   
   which is now replaced by "markUsed=true" + providing that interval ("2019-03-01/2019-04-01").




-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
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 #11501: Allow kill task to mark segments as unused

Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #11501:
URL: https://github.com/apache/druid/pull/11501#discussion_r677870532



##########
File path: server/src/main/java/org/apache/druid/client/indexing/HttpIndexingServiceClient.java
##########
@@ -70,7 +70,7 @@ public HttpIndexingServiceClient(
   public void killUnusedSegments(String idPrefix, String dataSource, Interval interval)

Review comment:
       I'm not sure what this class is used for since there arent any docs about it, but will adding the parameter here instead of defaulting to false break anything? If not, any reason not to add?




-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org