You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "abhishekrb19 (via GitHub)" <gi...@apache.org> on 2024/02/28 08:49:23 UTC
[PR] Kill task version support (druid)
abhishekrb19 opened a new pull request, #15994:
URL: https://github.com/apache/druid/pull/15994
By default, kill tasks delete all versions of unused segments within the specified interval. Users who wish to delete specific versions, for example, data compliance reasons, while retaining the rest, can specify an optional `version` in the kill task payload. If left unspecified, the default behavior remains unchanged, i.e., delete all versions in the interval.
Note that marking segments as used/unused by an optional version would be a complementary addition. I didn't make those changes in this PR to keep it simple to review - I will follow up on that change later.
<!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
#### Release note
<!-- Give your best effort to summarize your changes in a couple of sentences aimed toward Druid users.
If your change doesn't have end user impact, you can skip this section.
For tips about how to write a good release note, see [Release notes](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#release-notes).
-->
Kill task accepts an optional `version` delete a specific set of unused segments.
<hr>
##### Key changed/added classes in this PR
* `KillUnusedSegmentsTask.java`
* `RetrieveUnusedSegmentsAction.java`
* `RetrieveUsedSegmentsAction.java`
* `KillUnusedSegmentsTaskTest.java`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [x] been self-reviewed.
- [x] added documentation for new or modified features or behaviors.
- [x] a release note entry in the PR description.
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [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.
- [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
Re: [PR] Kill segments by versions (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1519243525
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/RetrieveUnusedSegmentsAction.java:
##########
@@ -40,6 +40,9 @@ public class RetrieveUnusedSegmentsAction implements TaskAction<List<DataSegment
@JsonIgnore
private final Interval interval;
+ @JsonIgnore
Review Comment:
Do all of these fields need the `@JsonIgnore` tag? Seems a little misleading since the fields are actually meant to be serialized.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -83,6 +84,11 @@ public class KillUnusedSegmentsTask extends AbstractFixedIntervalTask
*/
private static final int DEFAULT_SEGMENT_NUKE_BATCH_SIZE = 100;
+ /**
+ * The version of segments to delete in this {@link #getInterval()}.
+ */
+ @Nullable private final List<String> versions;
Review Comment:
```suggestion
@Nullable
private final List<String> versions;
```
##########
server/src/main/java/org/apache/druid/indexing/overlord/IndexerMetadataStorageCoordinator.java:
##########
@@ -124,6 +127,7 @@ default Collection<DataSegment> retrieveUsedSegmentsForInterval(
Collection<DataSegment> retrieveUsedSegmentsForIntervals(
Review Comment:
This interface is not marked `@PublicApi` or otherwise exposed as an extension point. So it is safe to update methods as needed.
But, as @zachjsh suggests, I would prefer we add new methods here and anywhere else if applicable. We seem to be passing a lot of nulls right now from several places just because there is one usage of the method that needs to pass a non-null `versions`.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -131,10 +138,22 @@ public KillUnusedSegmentsTask(
if (limit != null && Boolean.TRUE.equals(markAsUnused)) {
throw InvalidInput.exception("limit[%d] cannot be provided when markAsUnused is enabled.", limit);
}
+ if (!CollectionUtils.isNullOrEmpty(versions) && Boolean.TRUE.equals(markAsUnused)) {
+ throw InvalidInput.exception("versions[%s] cannot be provided when markAsUnused is enabled.", versions);
+ }
Review Comment:
Maybe put both of these inside a single if,
```java
if (Boolean.TRUE.equals(markAsUnused)) {
// if versions is not null/empty, do something
// if limit is not null, do something
}
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/RetrieveUsedSegmentsAction.java:
##########
@@ -73,6 +73,9 @@ public class RetrieveUsedSegmentsAction implements TaskAction<Collection<DataSeg
@JsonIgnore
private final List<Interval> intervals;
+ @JsonIgnore
+ private final List<String> versions;
Review Comment:
This action does not need a list of versions. Please see the comment on `KillUnusedSegmentsTask`.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -207,20 +226,17 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
@Nullable Integer numTotalBatches = getNumTotalBatches();
List<DataSegment> unusedSegments;
LOG.info(
- "Starting kill for datasource[%s] in interval[%s] with batchSize[%d], up to limit[%d] segments "
- + "before maxUsedStatusLastUpdatedTime[%s] will be deleted%s",
- getDataSource(),
- getInterval(),
- batchSize,
- limit,
- maxUsedStatusLastUpdatedTime,
+ "Starting kill for datasource[%s] in interval[%s] and versions[%s] with batchSize[%d], up to limit[%d]"
+ + " segments before maxUsedStatusLastUpdatedTime[%s] will be deleted%s",
+ getDataSource(), getInterval(), getVersions(), batchSize, limit, maxUsedStatusLastUpdatedTime,
numTotalBatches != null ? StringUtils.format(" in [%d] batches.", numTotalBatches) : "."
);
RetrieveUsedSegmentsAction retrieveUsedSegmentsAction = new RetrieveUsedSegmentsAction(
getDataSource(),
null,
ImmutableList.of(getInterval()),
+ getVersions(),
Review Comment:
The list of used segments should not be filtered on versions, only interval. It is possible that segments outside of the given list of versions may be using these load specs.
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1520857627
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java:
##########
@@ -619,6 +639,13 @@ private Query<Map<String, Object>> buildSegmentsTableQuery(
appendConditionForIntervalsAndMatchMode(sb, intervals, matchMode, connector);
}
+ if (!CollectionUtils.isNullOrEmpty(versions)) {
+ final String versionsStr = versions.stream()
+ .map(version -> "'" + version + "'")
+ .collect(Collectors.joining(","));
+ sb.append(StringUtils.format(" AND version IN (%s)", versionsStr));
Review Comment:
Good point. However, we don't enforce an upper bound on some related parameters like `limit` and `batchSize`, so I'm unsure if we want to do it for `versions` size. Also, the maximum size for `IN` would also depend on the underlying metadata store itself, so my suggestion would be to roll this out without any size restriction right out of the bat, and revisit this later if needed
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1519431702
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -207,20 +226,17 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
@Nullable Integer numTotalBatches = getNumTotalBatches();
List<DataSegment> unusedSegments;
LOG.info(
- "Starting kill for datasource[%s] in interval[%s] with batchSize[%d], up to limit[%d] segments "
- + "before maxUsedStatusLastUpdatedTime[%s] will be deleted%s",
- getDataSource(),
- getInterval(),
- batchSize,
- limit,
- maxUsedStatusLastUpdatedTime,
+ "Starting kill for datasource[%s] in interval[%s] and versions[%s] with batchSize[%d], up to limit[%d]"
+ + " segments before maxUsedStatusLastUpdatedTime[%s] will be deleted%s",
+ getDataSource(), getInterval(), getVersions(), batchSize, limit, maxUsedStatusLastUpdatedTime,
numTotalBatches != null ? StringUtils.format(" in [%d] batches.", numTotalBatches) : "."
);
RetrieveUsedSegmentsAction retrieveUsedSegmentsAction = new RetrieveUsedSegmentsAction(
getDataSource(),
null,
ImmutableList.of(getInterval()),
+ getVersions(),
Review Comment:
Ah, I see, thank you for the clarification! I will remove versions from `RetrieveUsedSegmentsAction`.
--
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
Re: [PR] Kill segments by version (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1515514934
##########
server/src/main/java/org/apache/druid/indexing/overlord/IndexerMetadataStorageCoordinator.java:
##########
@@ -124,6 +127,7 @@ default Collection<DataSegment> retrieveUsedSegmentsForInterval(
Collection<DataSegment> retrieveUsedSegmentsForIntervals(
Review Comment:
They are only used by the overlord. The interface allows tests to have custom implementations as needed. Also, the new nullable `version` parameter shouldn't cause any issues with rolling upgrade and downgrade situations. @kfaraz, could you please spot check this assumption?
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1522442926
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTaskTest.java:
##########
@@ -569,10 +735,10 @@ public void testKillMultipleUnusedSegmentsWithDifferentMaxUsedStatusLastUpdatedT
final List<DataSegment> unusedSegments =
getMetadataStorageCoordinator().retrieveUnusedSegmentsForInterval(
- DATA_SOURCE,
- umbrellaInterval,
- null,
- null
+ DATA_SOURCE,
+ umbrellaInterval,
+ null,
+ null
);
Review Comment:
Indentation seems off. Closing brace should have smaller indentation than the preceding args.
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTaskTest.java:
##########
@@ -173,62 +227,168 @@ public void testKillUnusedSegmentsWithUsedLoadSpec() throws Exception
null
);
- Assert.assertEquals(ImmutableSet.of(), new HashSet<>(observedUnusedSegments));
+ Assert.assertEquals(ImmutableSet.of(segment5), new HashSet<>(observedUnusedSegments));
}
-
@Test
- public void testKillWithMarkUnused() throws Exception
+ public void testKillSegmentsWithVersionsAndLimit() 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 DateTime now = DateTimes.nowUtc();
+ final String v1 = now.toString();
+ final String v2 = now.minusHours(2).toString();
+ final String v3 = now.minusHours(3).toString();
+
+ final DataSegment segment1 = newSegment(Intervals.of("2019-01-01/2019-02-01"), v1);
+ final DataSegment segment2 = newSegment(Intervals.of("2019-02-01/2019-03-01"), v1);
+ final DataSegment segment3 = newSegment(Intervals.of("2019-03-01/2019-04-01"), v1);
+ final DataSegment segment4 = newSegment(Intervals.of("2019-01-01/2019-02-01"), v2);
+ final DataSegment segment5 = newSegment(Intervals.of("2019-01-01/2019-02-01"), v3);
+
+ final Set<DataSegment> segments = ImmutableSet.of(segment1, segment2, segment3, segment4, segment5);
+
+ Assert.assertEquals(segments, getMetadataStorageCoordinator().commitSegments(segments));
+ Assert.assertEquals(
+ segments.size(),
+ getSegmentsMetadataManager().markSegmentsAsUnused(
+ segments.stream().map(DataSegment::getId).collect(Collectors.toSet())
+ )
);
- final Set<DataSegment> announced = getMetadataStorageCoordinator().commitSegments(segments);
- Assert.assertEquals(segments, announced);
+ final KillUnusedSegmentsTask task = new KillUnusedSegmentsTask(
+ null,
+ DATA_SOURCE,
+ Intervals.of("2018/2020"),
+ ImmutableList.of(v1),
+ null,
+ false,
+ 3,
+ 2,
+ null
+ );
- Assert.assertTrue(
- getSegmentsMetadataManager().markSegmentAsUnused(
- newSegment(Intervals.of("2019-02-01/2019-03-01"), version).getId()
+ Assert.assertEquals(TaskState.SUCCESS, taskRunner.run(task).get().getStatusCode());
+ Assert.assertEquals(
+ new KillTaskReport.Stats(2, 1, 0),
+ getReportedStats()
+ );
+
+ final List<DataSegment> observedUnusedSegments = getMetadataStorageCoordinator().retrieveUnusedSegmentsForInterval(
+ DATA_SOURCE,
+ Intervals.of("2018/2020"),
+ null,
+ null
+ );
+
+ Assert.assertEquals(ImmutableSet.of(segment3, segment4, segment5), new HashSet<>(observedUnusedSegments));
+ }
+
+ @Test
+ public void testKillWithNonExistentVersion() throws Exception
+ {
+ final DateTime now = DateTimes.nowUtc();
+ final String v1 = now.toString();
+ final String v2 = now.minusHours(2).toString();
+ final String v3 = now.minusHours(3).toString();
+
+ final DataSegment segment1 = newSegment(Intervals.of("2019-01-01/2019-02-01"), v1);
+ final DataSegment segment2 = newSegment(Intervals.of("2019-02-01/2019-03-01"), v1);
+ final DataSegment segment3 = newSegment(Intervals.of("2019-03-01/2019-04-01"), v1);
+ final DataSegment segment4 = newSegment(Intervals.of("2019-01-01/2019-02-01"), v2);
+ final DataSegment segment5 = newSegment(Intervals.of("2019-01-01/2019-02-01"), v3);
+
+ final Set<DataSegment> segments = ImmutableSet.of(segment1, segment2, segment3, segment4, segment5);
+
+ Assert.assertEquals(segments, getMetadataStorageCoordinator().commitSegments(segments));
+ Assert.assertEquals(
+ segments.size(),
+ getSegmentsMetadataManager().markSegmentsAsUnused(
+ segments.stream().map(DataSegment::getId).collect(Collectors.toSet())
)
);
- final KillUnusedSegmentsTask task =
- new KillUnusedSegmentsTask(
- null,
- DATA_SOURCE,
- Intervals.of("2019-03-01/2019-04-01"),
- null,
- true,
- null,
- null,
- null
- );
+ final KillUnusedSegmentsTask task = new KillUnusedSegmentsTask(
+ null,
+ DATA_SOURCE,
+ Intervals.of("2018/2020"),
+ ImmutableList.of(now.plusDays(100).toString()),
+ null,
+ false,
+ 3,
+ 2,
+ null
+ );
Assert.assertEquals(TaskState.SUCCESS, taskRunner.run(task).get().getStatusCode());
+ Assert.assertEquals(
+ new KillTaskReport.Stats(0, 1, 0),
+ getReportedStats()
+ );
- final List<DataSegment> unusedSegments =
- getMetadataStorageCoordinator().retrieveUnusedSegmentsForInterval(
- DATA_SOURCE,
- Intervals.of("2019/2020"),
- null,
- null
- );
+ final List<DataSegment> observedUnusedSegments = getMetadataStorageCoordinator().retrieveUnusedSegmentsForInterval(
+ DATA_SOURCE,
+ Intervals.of("2018/2020"),
+ null,
+ null
+ );
- 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)
+ Assert.assertEquals(segments, new HashSet<>(observedUnusedSegments));
+ }
+
+ /**
+ * {@code segment1}, {@code segment2} and {@code segment3} have different versions, but share the same load spec.
+ * {@code segment1} and {@code segment2} are unused segments, while {@code segment3} is a used segment.
+ * When a kill task is submitted, the unused segments {@code segment1} and {@code segment2} should be deleted from the
+ * metadata store, but should be retained in deep storage as the load spec is used by {@code segment3}.
+ */
+ @Test
+ public void testKillUnusedSegmentsWithUsedLoadSpec() throws Exception
+ {
+ final DateTime now = DateTimes.nowUtc();
+ final String v1 = now.toString();
+ final String v2 = now.minusHours(2).toString();
+ final String v3 = now.minusHours(3).toString();
+
+ final DataSegment segment1 = newSegment(Intervals.of("2019-01-01/2019-02-01"), v1, ImmutableMap.of("foo", "1"));
+ final DataSegment segment2 = newSegment(Intervals.of("2019-02-01/2019-03-01"), v2, ImmutableMap.of("foo", "1"));
+ final DataSegment segment3 = newSegment(Intervals.of("2019-03-01/2019-04-01"), v3, ImmutableMap.of("foo", "1"));
Review Comment:
Many of the tests seem to be using similar segments, versions, etc. Do you think some of this can go into the `setup` method?
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTaskTest.java:
##########
@@ -101,10 +102,11 @@ public void testKill() throws Exception
Assert.assertEquals(TaskState.SUCCESS, taskRunner.run(task).get().getStatusCode());
final List<DataSegment> unusedSegments = getMetadataStorageCoordinator().retrieveUnusedSegmentsForInterval(
- DATA_SOURCE,
- Intervals.of("2019/2020"),
- null,
- null
+ DATA_SOURCE,
+ Intervals.of("2019/2020"),
+ null,
+ null,
+ null
);
Review Comment:
Indentation seems off when looking at the args in the previous lines.
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1519568883
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/RetrieveUnusedSegmentsAction.java:
##########
@@ -40,6 +40,9 @@ public class RetrieveUnusedSegmentsAction implements TaskAction<List<DataSegment
@JsonIgnore
private final Interval interval;
+ @JsonIgnore
Review Comment:
Good question. On second thought, it does look confusing. I opened a PR to remove these annotations from the different task action classes - https://github.com/apache/druid/pull/16099
--
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
Re: [PR] Kill segments by version (druid)
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1509518100
##########
indexing-service/src/test/java/org/apache/druid/indexing/test/TestIndexerMetadataStorageCoordinator.java:
##########
@@ -98,7 +98,7 @@ public List<Pair<DataSegment, String>> retrieveUsedSegmentsAndCreatedDates(Strin
public List<DataSegment> retrieveUsedSegmentsForIntervals(
String dataSource,
List<Interval> intervals,
- Segments visibility
+ String version, Segments visibility
Review Comment:
nit: formatting; each parameter on its own line.
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 merged PR #15994:
URL: https://github.com/apache/druid/pull/15994
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1519568883
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/RetrieveUnusedSegmentsAction.java:
##########
@@ -40,6 +40,9 @@ public class RetrieveUnusedSegmentsAction implements TaskAction<List<DataSegment
@JsonIgnore
private final Interval interval;
+ @JsonIgnore
Review Comment:
Good question. On second thought, it's confusing. I opened a PR to remove these annotations from the different task action classes - https://github.com/apache/druid/pull/16099
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1519642996
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/RetrieveUnusedSegmentsAction.java:
##########
@@ -40,6 +40,9 @@ public class RetrieveUnusedSegmentsAction implements TaskAction<List<DataSegment
@JsonIgnore
private final Interval interval;
+ @JsonIgnore
Review Comment:
Yeah, it's going to conflict with https://github.com/apache/druid/pull/16099, so I will merge that change in this branch and then remove it here
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1519313320
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -207,20 +226,17 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
@Nullable Integer numTotalBatches = getNumTotalBatches();
List<DataSegment> unusedSegments;
LOG.info(
- "Starting kill for datasource[%s] in interval[%s] with batchSize[%d], up to limit[%d] segments "
- + "before maxUsedStatusLastUpdatedTime[%s] will be deleted%s",
- getDataSource(),
- getInterval(),
- batchSize,
- limit,
- maxUsedStatusLastUpdatedTime,
+ "Starting kill for datasource[%s] in interval[%s] and versions[%s] with batchSize[%d], up to limit[%d]"
+ + " segments before maxUsedStatusLastUpdatedTime[%s] will be deleted%s",
+ getDataSource(), getInterval(), getVersions(), batchSize, limit, maxUsedStatusLastUpdatedTime,
numTotalBatches != null ? StringUtils.format(" in [%d] batches.", numTotalBatches) : "."
);
RetrieveUsedSegmentsAction retrieveUsedSegmentsAction = new RetrieveUsedSegmentsAction(
getDataSource(),
null,
ImmutableList.of(getInterval()),
+ getVersions(),
Review Comment:
@kfaraz - sure, can you elaborate on this line for clarity:
>It is possible that segments outside of the given list of versions may be using these load specs.
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1519397596
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -207,20 +226,17 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
@Nullable Integer numTotalBatches = getNumTotalBatches();
List<DataSegment> unusedSegments;
LOG.info(
- "Starting kill for datasource[%s] in interval[%s] with batchSize[%d], up to limit[%d] segments "
- + "before maxUsedStatusLastUpdatedTime[%s] will be deleted%s",
- getDataSource(),
- getInterval(),
- batchSize,
- limit,
- maxUsedStatusLastUpdatedTime,
+ "Starting kill for datasource[%s] in interval[%s] and versions[%s] with batchSize[%d], up to limit[%d]"
+ + " segments before maxUsedStatusLastUpdatedTime[%s] will be deleted%s",
+ getDataSource(), getInterval(), getVersions(), batchSize, limit, maxUsedStatusLastUpdatedTime,
numTotalBatches != null ? StringUtils.format(" in [%d] batches.", numTotalBatches) : "."
);
RetrieveUsedSegmentsAction retrieveUsedSegmentsAction = new RetrieveUsedSegmentsAction(
getDataSource(),
null,
ImmutableList.of(getInterval()),
+ getVersions(),
Review Comment:
Sure, @abhishekrb19 .
So, we fetch the set of used segments here to ensure that we do not end up killing a segment whose load spec is still in use.
As a result of the segment upgrade logic introduced in PR #14407 , there can be multiple segment IDs belonging to different versions that refer to the same load spec i.e. the same segment folder on deep storage. So if a segment ID belong to version0 is now unused, the actual physical segment may still be needed by some other segment ID which belongs to version1.
Hope that clarifies things. In conclusion, we shouldn't need to specify versions while retrieving __used__ segments.
--
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
Re: [PR] Kill task version support (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1505579554
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -131,8 +137,12 @@ public KillUnusedSegmentsTask(
if (limit != null && Boolean.TRUE.equals(markAsUnused)) {
throw InvalidInput.exception("limit[%d] cannot be provided when markAsUnused is enabled.", limit);
}
+ if (version != null && Boolean.TRUE.equals(markAsUnused)) {
+ throw InvalidInput.exception("version[%s] cannot be provided when markAsUnused is enabled.", version);
Review Comment:
I didn't add mark used/unused support for versions in this patch to keep the changes easy to review. Also, the `markAsUnused` option is deprecated, so maybe it's ok to keep it this way?
--
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
Re: [PR] Kill segments by version (druid)
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1509519867
##########
server/src/main/java/org/apache/druid/indexing/overlord/IndexerMetadataStorageCoordinator.java:
##########
@@ -124,6 +127,7 @@ default Collection<DataSegment> retrieveUsedSegmentsForInterval(
Collection<DataSegment> retrieveUsedSegmentsForIntervals(
Review Comment:
Can these methods be used by third party extensions, which would fail compilation with the change in parameters. Should we instead add a new version of the function?
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1519609784
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/actions/RetrieveSegmentsActionsTest.java:
##########
@@ -98,31 +102,37 @@ public void testRetrieveUsedSegmentsAction()
{
final RetrieveUsedSegmentsAction action =
new RetrieveUsedSegmentsAction(task.getDataSource(), ImmutableList.of(INTERVAL));
- final Set<DataSegment> resultSegments = new HashSet<>(action.perform(task, actionTestKit.getTaskActionToolbox()));
- Assert.assertEquals(expectedUsedSegments, resultSegments);
+ final Set<DataSegment> actualSegments = new HashSet<>(action.perform(task, actionTestKit.getTaskActionToolbox()));
Review Comment:
Suggestion: I typically use nomenclature of `expected` vs `observed`, e.g. `observedUsedSegments`.
##########
server/src/main/java/org/apache/druid/client/indexing/ClientKillUnusedSegmentsTaskQuery.java:
##########
@@ -40,6 +41,7 @@ public class ClientKillUnusedSegmentsTaskQuery implements ClientTaskQuery
private final String id;
private final String dataSource;
private final Interval interval;
+ @Nullable private final List<String> versions;
Review Comment:
```suggestion
@Nullable
private final List<String> versions;
```
##########
server/src/main/java/org/apache/druid/client/indexing/ClientKillUnusedSegmentsTaskQuery.java:
##########
@@ -50,6 +52,7 @@ public ClientKillUnusedSegmentsTaskQuery(
@JsonProperty("id") String id,
@JsonProperty("dataSource") String dataSource,
@JsonProperty("interval") Interval interval,
+ @JsonProperty("versions") List<String> versions,
Review Comment:
```suggestion
@JsonProperty("versions") @Nullable List<String> versions,
```
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/RetrieveUnusedSegmentsAction.java:
##########
@@ -40,6 +40,9 @@ public class RetrieveUnusedSegmentsAction implements TaskAction<List<DataSegment
@JsonIgnore
private final Interval interval;
+ @JsonIgnore
Review Comment:
So, you plan to remove the `@JsonIgnore` from here too, right?
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1521118358
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTaskTest.java:
##########
@@ -593,14 +760,113 @@ public void testKillMultipleUnusedSegmentsWithDifferentMaxUsedStatusLastUpdatedT
Assert.assertEquals(TaskState.SUCCESS, taskRunner.run(task2).get().getStatusCode());
final List<DataSegment> unusedSegments2 = getMetadataStorageCoordinator().retrieveUnusedSegmentsForInterval(
+ DATA_SOURCE,
+ umbrellaInterval,
+ null,
+ null
+ );
+
+ Assert.assertEquals(ImmutableList.of(), unusedSegments2);
+ Assert.assertEquals(new KillTaskReport.Stats(2, 3, 0), getReportedStats());
+ }
+
+ @Test
+ public void testKillMultipleUnusedSegmentsWithVersionAndDifferentLastUpdatedTime() throws Exception
+ {
+ final DateTime version = DateTimes.nowUtc();
+ final DataSegment segment1 = newSegment(Intervals.of("2019-01-01/2019-02-01"), version.toString());
+ final DataSegment segment2 = newSegment(Intervals.of("2019-02-01/2019-03-01"), version.toString());
+ final DataSegment segment3 = newSegment(Intervals.of("2019-03-01/2019-04-01"), version.toString());
+ final DataSegment segment4 = newSegment(Intervals.of("2019-04-01/2019-05-01"), version.minusHours(2).toString());
+ final DataSegment segment5 = newSegment(Intervals.of("2019-04-01/2019-05-01"), version.minusHours(3).toString());
+
+ final Set<DataSegment> segments = ImmutableSet.of(segment1, segment2, segment3, segment4, segment5);
+ Assert.assertEquals(segments, getMetadataStorageCoordinator().commitSegments(segments));
+
+ Assert.assertEquals(
+ 3,
+ getSegmentsMetadataManager().markSegmentsAsUnused(
+ ImmutableSet.of(segment1.getId(), segment2.getId(), segment4.getId())
+ )
+ );
+
+ // Capture the last updated time cutoff
+ final DateTime maxUsedStatusLastUpdatedTime1 = DateTimes.nowUtc();
+
+ // Delay for 1s, mark the segments as unused and then capture the last updated time cutoff again
+ Thread.sleep(1000);
Review Comment:
I think we can actually remove the sleep here and in the other existing tests by directly marking the segments as unused using the test connector. That way, the test would have control over the last updated time and we can set it to whatever time. I can clean up this pattern along with other miscellaneous testing stuff in a follow-up patch
--
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
Re: [PR] Kill segments by version (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1515508935
##########
server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java:
##########
@@ -1068,6 +1068,7 @@ public void testMultiIntervalUsedList() throws IOException
coordinator.retrieveUsedSegmentsForIntervals(
defaultSegment.getDataSource(),
ImmutableList.of(defaultSegment.getInterval()),
+ null,
Review Comment:
Thanks. Added a few unit tests in this test as well. `RetrieveSegmentsActionsTest` has coverage of this functionality too.
--
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
Re: [PR] Kill task version support (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1505579554
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -131,8 +137,12 @@ public KillUnusedSegmentsTask(
if (limit != null && Boolean.TRUE.equals(markAsUnused)) {
throw InvalidInput.exception("limit[%d] cannot be provided when markAsUnused is enabled.", limit);
}
+ if (version != null && Boolean.TRUE.equals(markAsUnused)) {
+ throw InvalidInput.exception("version[%s] cannot be provided when markAsUnused is enabled.", version);
Review Comment:
I didn't change the mark used/unused APIs to support version in this patch to keep the changes easy to review. Also, the `markAsUnused` option is deprecated, so maybe it's ok to keep it this way?
--
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
Re: [PR] Kill segments by version (druid)
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1509508297
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -131,8 +137,12 @@ public KillUnusedSegmentsTask(
if (limit != null && Boolean.TRUE.equals(markAsUnused)) {
throw InvalidInput.exception("limit[%d] cannot be provided when markAsUnused is enabled.", limit);
}
+ if (version != null && Boolean.TRUE.equals(markAsUnused)) {
+ throw InvalidInput.exception("version[%s] cannot be provided when markAsUnused is enabled.", version);
Review Comment:
think this is ok 👌
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -131,8 +137,12 @@ public KillUnusedSegmentsTask(
if (limit != null && Boolean.TRUE.equals(markAsUnused)) {
throw InvalidInput.exception("limit[%d] cannot be provided when markAsUnused is enabled.", limit);
}
+ if (version != null && Boolean.TRUE.equals(markAsUnused)) {
+ throw InvalidInput.exception("version[%s] cannot be provided when markAsUnused is enabled.", version);
Review Comment:
I think this is ok 👌
--
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
Re: [PR] Kill segments by version (druid)
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1509525944
##########
server/src/main/java/org/apache/druid/indexing/overlord/IndexerMetadataStorageCoordinator.java:
##########
@@ -133,18 +137,20 @@ Collection<DataSegment> retrieveUsedSegmentsForIntervals(
*
* @param dataSource The data source the segments belong to
* @param interval Filter the data segments to ones that include data in this interval exclusively.
+ * @param version An optional version of segments to retrieve in the given {@code interval}. If unspecified, all
+ * versions of unused segments in the {@code interval} must be retrieved.
* @param limit The maximum number of unused segments to retreive. If null, no limit is applied.
* @param maxUsedStatusLastUpdatedTime The maximum {@code used_status_last_updated} time. Any unused segment in {@code interval}
- * with {@code used_status_last_updated} no later than this time will be included in the
- * kill task. Segments without {@code used_status_last_updated} time (due to an upgrade
- * from legacy Druid) will have {@code maxUsedStatusLastUpdatedTime} ignored
- *
+ * with {@code used_status_last_updated} no later than this time will be included in the
+ * kill task. Segments without {@code used_status_last_updated} time (due to an upgrade
+ * from legacy Druid) will have {@code maxUsedStatusLastUpdatedTime} ignored
* @return DataSegments which include ONLY data within the requested interval and are marked as unused. Segments NOT
* returned here may include data in the interval
*/
List<DataSegment> retrieveUnusedSegmentsForInterval(
Review Comment:
ditto
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1519321119
##########
server/src/main/java/org/apache/druid/indexing/overlord/IndexerMetadataStorageCoordinator.java:
##########
@@ -124,6 +127,7 @@ default Collection<DataSegment> retrieveUsedSegmentsForInterval(
Collection<DataSegment> retrieveUsedSegmentsForIntervals(
Review Comment:
👍 I can set the existing interface method as the default and invoke the new method inside it, so null is passed only from the default implementation.
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1520798465
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java:
##########
@@ -619,6 +639,13 @@ private Query<Map<String, Object>> buildSegmentsTableQuery(
appendConditionForIntervalsAndMatchMode(sb, intervals, matchMode, connector);
}
+ if (!CollectionUtils.isNullOrEmpty(versions)) {
+ final String versionsStr = versions.stream()
+ .map(version -> "'" + version + "'")
+ .collect(Collectors.joining(","));
+ sb.append(StringUtils.format(" AND version IN (%s)", versionsStr));
Review Comment:
should we enforce some limit on the number of versions here? I think in practice most users wouldnt be specifying too many versions at once.
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1521118358
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTaskTest.java:
##########
@@ -593,14 +760,113 @@ public void testKillMultipleUnusedSegmentsWithDifferentMaxUsedStatusLastUpdatedT
Assert.assertEquals(TaskState.SUCCESS, taskRunner.run(task2).get().getStatusCode());
final List<DataSegment> unusedSegments2 = getMetadataStorageCoordinator().retrieveUnusedSegmentsForInterval(
+ DATA_SOURCE,
+ umbrellaInterval,
+ null,
+ null
+ );
+
+ Assert.assertEquals(ImmutableList.of(), unusedSegments2);
+ Assert.assertEquals(new KillTaskReport.Stats(2, 3, 0), getReportedStats());
+ }
+
+ @Test
+ public void testKillMultipleUnusedSegmentsWithVersionAndDifferentLastUpdatedTime() throws Exception
+ {
+ final DateTime version = DateTimes.nowUtc();
+ final DataSegment segment1 = newSegment(Intervals.of("2019-01-01/2019-02-01"), version.toString());
+ final DataSegment segment2 = newSegment(Intervals.of("2019-02-01/2019-03-01"), version.toString());
+ final DataSegment segment3 = newSegment(Intervals.of("2019-03-01/2019-04-01"), version.toString());
+ final DataSegment segment4 = newSegment(Intervals.of("2019-04-01/2019-05-01"), version.minusHours(2).toString());
+ final DataSegment segment5 = newSegment(Intervals.of("2019-04-01/2019-05-01"), version.minusHours(3).toString());
+
+ final Set<DataSegment> segments = ImmutableSet.of(segment1, segment2, segment3, segment4, segment5);
+ Assert.assertEquals(segments, getMetadataStorageCoordinator().commitSegments(segments));
+
+ Assert.assertEquals(
+ 3,
+ getSegmentsMetadataManager().markSegmentsAsUnused(
+ ImmutableSet.of(segment1.getId(), segment2.getId(), segment4.getId())
+ )
+ );
+
+ // Capture the last updated time cutoff
+ final DateTime maxUsedStatusLastUpdatedTime1 = DateTimes.nowUtc();
+
+ // Delay for 1s, mark the segments as unused and then capture the last updated time cutoff again
+ Thread.sleep(1000);
Review Comment:
I think we can actually remove the sleep here and in the other existing tests by directly marking the segments as unused using the test connector. That way, the test would have control over the last updated time and we can set it to whatever time. I can clean up this pattern in a follow-up patch
--
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
Re: [PR] Kill segments by version (druid)
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1509530325
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1456,7 +1462,7 @@ private Set<DataSegment> createNewIdsForAppendSegments(
final Collection<DataSegment> overlappingSegments = retrieveUsedSegmentsForIntervals(
dataSource,
new ArrayList<>(appendIntervals),
- Segments.INCLUDING_OVERSHADOWED
+ null, Segments.INCLUDING_OVERSHADOWED
Review Comment:
nit: formatting
--
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
Re: [PR] Kill segments by version (druid)
Posted by "zachjsh (via GitHub)" <gi...@apache.org>.
zachjsh commented on code in PR #15994:
URL: https://github.com/apache/druid/pull/15994#discussion_r1509535701
##########
server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java:
##########
@@ -1068,6 +1068,7 @@ public void testMultiIntervalUsedList() throws IOException
coordinator.retrieveUsedSegmentsForIntervals(
defaultSegment.getDataSource(),
ImmutableList.of(defaultSegment.getInterval()),
+ null,
Review Comment:
Should we add tests here that test with version specified, or does something else exercise that code path?
--
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
Re: [PR] Kill segments by versions (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on PR #15994:
URL: https://github.com/apache/druid/pull/15994#issuecomment-1993411560
Thanks for the thorough reviews, @kfaraz!
> Thanks for the changes, @abhishekrb19 !
The test improvements can be done in a follow up.
>Out of curiosity, do you plan to make similar changes for markUsed / markUnused APIs too?
Yes, I'm going to do similar changes to the markUsed / markUnused APIs next. I will also follow up on the test improvements in a separate patch.
--
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