You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kfaraz (via GitHub)" <gi...@apache.org> on 2026/07/23 17:33:43 UTC
[PR] fix: Stop kill task if parent IDs of segment could not be determined (druid)
kfaraz opened a new pull request, #19737:
URL: https://github.com/apache/druid/pull/19737
### Changes
- Stop `kill` task (and embedded kill tasks) if parent IDs of the killable unused segments could not be determined. There are 2 cases possible:
- Segment does not share load spec: Cannot remove segment from metadata store, otherwise deep store files would become orphaned and will never be removed.
- Segment shares load spec with other segments: Cannot remove segment files from deep store as the same files are needed by other segments.
- Avoid extra DB query to fetch parent IDs in case of embedded kill, by pre-fetching the parent IDs in the previous call which identifies the killable unused segments in the first place.
<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:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] 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.
- [ ] 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.
- [ ] 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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3640878801
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -387,13 +434,12 @@ private NavigableMap<DateTime, List<TaskLock>> getNonRevokedTaskLockMap(TaskActi
* @return list of segments to kill from deep storage
*/
private List<DataSegment> getKillableSegments(
- List<DataSegment> unusedSegments,
+ Set<DataSegment> unusedSegments,
Review Comment:
Since this method is now invoked before metadata entries have been nuked, it should exclude self segment ID while doing the sibling check.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -387,13 +434,12 @@ private NavigableMap<DateTime, List<TaskLock>> getNonRevokedTaskLockMap(TaskActi
* @return list of segments to kill from deep storage
*/
private List<DataSegment> getKillableSegments(
- List<DataSegment> unusedSegments,
+ Set<DataSegment> unusedSegments,
Review Comment:
TODO: Since this method is now invoked before metadata entries have been nuked, it should exclude self segment ID while doing the sibling check.
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3640954399
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -387,13 +434,12 @@ private NavigableMap<DateTime, List<TaskLock>> getNonRevokedTaskLockMap(TaskActi
* @return list of segments to kill from deep storage
*/
private List<DataSegment> getKillableSegments(
- List<DataSegment> unusedSegments,
+ Set<DataSegment> unusedSegments,
Review Comment:
done
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3649556260
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -87,6 +85,10 @@
* <li> Filter the set of unreferenced segments using load specs from the set of used segments. </li>
* <li> Kill the filtered set of segments from deep storage. </li>
* </ol>
+ * Note: When {@link Tasks#USE_CONCURRENT_LOCKS} is true, keep a large buffer
+ * period before killing segments after they have been marked as unused.
+ * Otherwise, there may be a potential data loss if a concurrent APPEND job
Review Comment:
I think it'd be good to document this in the kill section of the docs as well: https://druid.apache.org/docs/latest/data-management/delete/
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -236,12 +238,20 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
break;
}
- unusedSegments = fetchNextBatchOfUnusedSegments(toolbox, nextBatchSize);
+ unusedSegmentsPlus = fetchNextBatchOfUnusedSegments(toolbox, nextBatchSize);
+ if (unusedSegmentsPlus.isEmpty()) {
+ // No more segments eligible for kill, do not proceed further
+ break;
+ }
// Fetch locks each time as a revokal could have occurred in between batches
final NavigableMap<DateTime, List<TaskLock>> taskLockMap
= getNonRevokedTaskLockMap(toolbox.getTaskActionClient());
+ final Set<DataSegment> unusedSegments = unusedSegmentsPlus.stream()
+ .map(DataSegmentPlus::getDataSegment)
+ .collect(Collectors.toSet());
+
if (!TaskLocks.isLockCoversSegments(taskLockMap, unusedSegments)) {
Review Comment:
nit: perhaps we could introduce a thin wrapper for `isLockCoversSegments()` that also accepts `DataSegmentPlus` that delegates to isLockCoversSegments(NavigableMap<DateTime, List<TaskLock>>, Collection<DataSegment> segments) to avoid another iteration over the set of segments.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -352,7 +346,47 @@ protected List<DataSegment> fetchNextBatchOfUnusedSegments(TaskToolbox toolbox,
nextBatchSize,
maxUsedStatusLastUpdatedTime
)
- );
+ )
+ .stream()
+ .map(segment -> new DataSegmentPlus(segment, null, null, null, null, null, null, null))
+ .collect(Collectors.toList());
+ }
+
+ /**
+ * Fetches the parent IDs (if any) for the given unused segments.
+ *
+ * @param unusedSegments Unused segments whose parent IDs need to be fetched
+ * @return Map from segment ID to the segment ID from which
+ * it was upgraded. If an input segment was not upgraded from any other segment,
+ * it does not have an entry in the map.
+ */
+ protected Map<String, String> fetchParentIdsForSegments(
+ TaskToolbox toolbox,
+ List<DataSegmentPlus> unusedSegments
+ )
+ {
+ try {
+ final Set<String> segmentIds = unusedSegments.stream().map(
+ s -> s.getDataSegment().getId().toString()
+ ).collect(Collectors.toSet());
Review Comment:
nit: We could compute this once in the caller and pass it to both this function and `getKillableSegments`.
##########
server/src/main/java/org/apache/druid/indexing/overlord/IndexerMetadataStorageCoordinator.java:
##########
@@ -161,10 +161,11 @@ List<DataSegment> retrieveUnusedSegmentsForInterval(
* @param maxUpdatedTime Returned segments must have a {@code used_status_last_updated}
* which is either null or earlier than this value.
* @param limit Maximum number of segments to return.
- *
* @return Unsorted list of unused segments that match the given parameters.
+ * The entries in the lost are required to have the {@link DataSegmentPlus#getDataSegment()}
Review Comment:
typo:
```suggestion
* The entries in the list are required to have the {@link DataSegmentPlus#getDataSegment()}
```
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3646645334
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -251,62 +258,46 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
);
}
- // Kill segments. Order is important here:
- // Retrieve the segment upgrade infos for the batch _before_ the segments are nuked
- // We then want the nuke action to clean up the metadata records _before_ the segments are removed from storage.
- // This helps maintain that we will always have a storage segment if the metadata segment is present.
- // Determine the subset of segments to be killed from deep storage based on loadspecs.
- // If the segment nuke throws an exception, then the segment cleanup is abandoned.
-
- // Determine upgraded segment ids before nuking
- final Set<String> segmentIds = unusedSegments.stream()
- .map(DataSegment::getId)
- .map(SegmentId::toString)
- .collect(Collectors.toSet());
- final Map<String, String> upgradedFromSegmentIds = new HashMap<>();
- try {
- upgradedFromSegmentIds.putAll(
- taskActionClient.submit(
- new RetrieveUpgradedFromSegmentIdsAction(getDataSource(), segmentIds)
- ).getUpgradedFromSegmentIds()
- );
- }
- catch (Exception e) {
- LOG.warn(
- e,
- "Could not retrieve parent segment ids using task action[retrieveUpgradedFromSegmentIds]."
- + " Overlord may be on an older version."
- );
- }
+ // Kill segments - order of steps 1, 2, 3, 4 must remain the same
- // Nuke Segments
- taskActionClient.submit(new SegmentNukeAction(new HashSet<>(unusedSegments)));
- emitMetric(toolbox.getEmitter(), TaskMetrics.SEGMENTS_DELETED_FROM_METADATA_STORE, unusedSegments.size());
+ // 1. Determine parent segment ids of killable unused segments
+ final Map<String, String> upgradedFromSegmentIds
+ = fetchParentIdsForSegments(toolbox, unusedSegmentsPlus);
- // Determine segments to be killed
- final List<DataSegment> segmentsToBeKilled
- = getKillableSegments(unusedSegments, upgradedFromSegmentIds, usedSegmentLoadSpecs, taskActionClient);
+ // 2. Identify killable segments whose load specs are not shared with any other segment
+ final List<DataSegment> segmentsToKillFromDeepStore = getKillableSegments(
+ unusedSegments,
+ upgradedFromSegmentIds,
+ usedSegmentLoadSpecs,
+ taskActionClient
+ );
+ // 2a. Track segments that cannot be removed from deep store yet
final Set<DataSegment> segmentsNotKilled = new HashSet<>(unusedSegments);
- segmentsToBeKilled.forEach(segmentsNotKilled::remove);
-
+ segmentsToKillFromDeepStore.forEach(segmentsNotKilled::remove);
if (!segmentsNotKilled.isEmpty()) {
LOG.warn(
- "Skipping kill of [%d] segments from deep storage as their load specs are used by other segments.",
- segmentsNotKilled.size()
+ "Skipping kill of [%d] segments of datasource[%s] from deep storage"
+ + " as their load specs are shared by other segments.",
+ segmentsNotKilled.size(), getDataSource()
);
}
- toolbox.getDataSegmentKiller().kill(segmentsToBeKilled);
- emitMetric(toolbox.getEmitter(), TaskMetrics.SEGMENTS_DELETED_FROM_DEEPSTORE, segmentsToBeKilled.size());
+ // 3. Nuke all eligible unused segments
Review Comment:
Adding a comment in javadoc.
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3650066514
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -352,7 +346,47 @@ protected List<DataSegment> fetchNextBatchOfUnusedSegments(TaskToolbox toolbox,
nextBatchSize,
maxUsedStatusLastUpdatedTime
)
- );
+ )
+ .stream()
+ .map(segment -> new DataSegmentPlus(segment, null, null, null, null, null, null, null))
+ .collect(Collectors.toList());
+ }
+
+ /**
+ * Fetches the parent IDs (if any) for the given unused segments.
+ *
+ * @param unusedSegments Unused segments whose parent IDs need to be fetched
+ * @return Map from segment ID to the segment ID from which
+ * it was upgraded. If an input segment was not upgraded from any other segment,
+ * it does not have an entry in the map.
+ */
+ protected Map<String, String> fetchParentIdsForSegments(
+ TaskToolbox toolbox,
+ List<DataSegmentPlus> unusedSegments
+ )
+ {
+ try {
+ final Set<String> segmentIds = unusedSegments.stream().map(
+ s -> s.getDataSegment().getId().toString()
+ ).collect(Collectors.toSet());
Review Comment:
Updated. Passing a `Map<String, DataSegmentPlus>` to these methods now.
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3644580678
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -352,7 +347,47 @@ protected List<DataSegment> fetchNextBatchOfUnusedSegments(TaskToolbox toolbox,
nextBatchSize,
maxUsedStatusLastUpdatedTime
)
- );
+ )
+ .stream()
+ .map(segment -> new DataSegmentPlus(segment, null, null, null, null, null, null, null))
+ .collect(Collectors.toList());
+ }
+
+ /**
+ * Fetches the parent IDs (if any) for the given unused segments.
+ *
+ * @param unusedSegments Unused segments whose parent IDs need to be fetched
+ * @return Map from segment ID to the segment ID from which
+ * it was upgraded. If an input segment was not upgraded from any other segment,
+ * it does not have an entry in the map.
+ */
+ protected Map<String, String> fetchParentIdsForSegments(
+ TaskToolbox toolbox,
+ List<DataSegmentPlus> unusedSegments
+ )
+ {
+ try {
+ final Set<String> segmentIds = unusedSegments.stream().map(
+ s -> s.getDataSegment().getId().toString()
+ ).collect(Collectors.toSet());
+
+ return toolbox.getTaskActionClient().submit(
+ new RetrieveUpgradedFromSegmentIdsAction(getDataSource(), segmentIds)
+ ).getUpgradedFromSegmentIds();
+ }
+ catch (Exception e) {
+ // Do not proceed with killing these segments as we cannot be sure if their
+ // load spec is shared by any other segment or not. If load spec is shared,
+ // segment files cannot be deleted from deep store. If load spec is not
+ // shared, segments cannot be deleted from metadata store as that would
+ // leave deep store files orphaned, and they would never be cleaned up.
+ throw new ISE(
+ e,
+ "Could not retrieve parent segment ids using task action[retrieveUpgradedFromSegmentIds]."
+ + " Stopping kill task to avoid data loss in case the segment files"
+ + " are shared by other segments."
Review Comment:
Thanks for the suggestion! Added some 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
Re: [PR] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3650049644
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -87,6 +85,10 @@
* <li> Filter the set of unreferenced segments using load specs from the set of used segments. </li>
* <li> Kill the filtered set of segments from deep storage. </li>
* </ol>
+ * Note: When {@link Tasks#USE_CONCURRENT_LOCKS} is true, keep a large buffer
+ * period before killing segments after they have been marked as unused.
+ * Otherwise, there may be a potential data loss if a concurrent APPEND job
Review Comment:
updated.
##########
server/src/main/java/org/apache/druid/indexing/overlord/IndexerMetadataStorageCoordinator.java:
##########
@@ -161,10 +161,11 @@ List<DataSegment> retrieveUnusedSegmentsForInterval(
* @param maxUpdatedTime Returned segments must have a {@code used_status_last_updated}
* which is either null or earlier than this value.
* @param limit Maximum number of segments to return.
- *
* @return Unsorted list of unused segments that match the given parameters.
+ * The entries in the lost are required to have the {@link DataSegmentPlus#getDataSegment()}
Review Comment:
fixed.
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3644210540
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -251,62 +258,50 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
);
}
- // Kill segments. Order is important here:
- // Retrieve the segment upgrade infos for the batch _before_ the segments are nuked
- // We then want the nuke action to clean up the metadata records _before_ the segments are removed from storage.
- // This helps maintain that we will always have a storage segment if the metadata segment is present.
- // Determine the subset of segments to be killed from deep storage based on loadspecs.
- // If the segment nuke throws an exception, then the segment cleanup is abandoned.
-
- // Determine upgraded segment ids before nuking
- final Set<String> segmentIds = unusedSegments.stream()
- .map(DataSegment::getId)
- .map(SegmentId::toString)
- .collect(Collectors.toSet());
- final Map<String, String> upgradedFromSegmentIds = new HashMap<>();
- try {
- upgradedFromSegmentIds.putAll(
- taskActionClient.submit(
- new RetrieveUpgradedFromSegmentIdsAction(getDataSource(), segmentIds)
- ).getUpgradedFromSegmentIds()
- );
- }
- catch (Exception e) {
- LOG.warn(
- e,
- "Could not retrieve parent segment ids using task action[retrieveUpgradedFromSegmentIds]."
- + " Overlord may be on an older version."
- );
- }
+ // Kill segments - order of steps 1, 2, 3, 4 must remain the same
- // Nuke Segments
- taskActionClient.submit(new SegmentNukeAction(new HashSet<>(unusedSegments)));
- emitMetric(toolbox.getEmitter(), TaskMetrics.SEGMENTS_DELETED_FROM_METADATA_STORE, unusedSegments.size());
+ // 1. Determine parent segment ids of killable unused segments
+ final Map<String, String> upgradedFromSegmentIds
+ = fetchParentIdsForSegments(toolbox, unusedSegmentsPlus);
- // Determine segments to be killed
- final List<DataSegment> segmentsToBeKilled
- = getKillableSegments(unusedSegments, upgradedFromSegmentIds, usedSegmentLoadSpecs, taskActionClient);
+ // 2. Identify killable segments whose load specs are not shared with any other segment
+ final List<DataSegment> segmentsToKillFromDeepStore = getKillableSegments(
+ unusedSegments,
+ upgradedFromSegmentIds,
+ usedSegmentLoadSpecs,
+ taskActionClient
+ );
+ // 2a. Track segments that cannot be removed from deep store yet
final Set<DataSegment> segmentsNotKilled = new HashSet<>(unusedSegments);
- segmentsToBeKilled.forEach(segmentsNotKilled::remove);
-
+ segmentsToKillFromDeepStore.forEach(segmentsNotKilled::remove);
if (!segmentsNotKilled.isEmpty()) {
LOG.warn(
- "Skipping kill of [%d] segments from deep storage as their load specs are used by other segments.",
- segmentsNotKilled.size()
+ "Skipping kill of [%d] segments of datasource[%s] from deep storage"
Review Comment:
I see. This makes sense as-is.
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3645238089
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -251,62 +258,46 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
);
}
- // Kill segments. Order is important here:
- // Retrieve the segment upgrade infos for the batch _before_ the segments are nuked
- // We then want the nuke action to clean up the metadata records _before_ the segments are removed from storage.
- // This helps maintain that we will always have a storage segment if the metadata segment is present.
- // Determine the subset of segments to be killed from deep storage based on loadspecs.
- // If the segment nuke throws an exception, then the segment cleanup is abandoned.
-
- // Determine upgraded segment ids before nuking
- final Set<String> segmentIds = unusedSegments.stream()
- .map(DataSegment::getId)
- .map(SegmentId::toString)
- .collect(Collectors.toSet());
- final Map<String, String> upgradedFromSegmentIds = new HashMap<>();
- try {
- upgradedFromSegmentIds.putAll(
- taskActionClient.submit(
- new RetrieveUpgradedFromSegmentIdsAction(getDataSource(), segmentIds)
- ).getUpgradedFromSegmentIds()
- );
- }
- catch (Exception e) {
- LOG.warn(
- e,
- "Could not retrieve parent segment ids using task action[retrieveUpgradedFromSegmentIds]."
- + " Overlord may be on an older version."
- );
- }
+ // Kill segments - order of steps 1, 2, 3, 4 must remain the same
- // Nuke Segments
- taskActionClient.submit(new SegmentNukeAction(new HashSet<>(unusedSegments)));
- emitMetric(toolbox.getEmitter(), TaskMetrics.SEGMENTS_DELETED_FROM_METADATA_STORE, unusedSegments.size());
+ // 1. Determine parent segment ids of killable unused segments
+ final Map<String, String> upgradedFromSegmentIds
+ = fetchParentIdsForSegments(toolbox, unusedSegmentsPlus);
- // Determine segments to be killed
- final List<DataSegment> segmentsToBeKilled
- = getKillableSegments(unusedSegments, upgradedFromSegmentIds, usedSegmentLoadSpecs, taskActionClient);
+ // 2. Identify killable segments whose load specs are not shared with any other segment
+ final List<DataSegment> segmentsToKillFromDeepStore = getKillableSegments(
+ unusedSegments,
+ upgradedFromSegmentIds,
+ usedSegmentLoadSpecs,
+ taskActionClient
+ );
+ // 2a. Track segments that cannot be removed from deep store yet
final Set<DataSegment> segmentsNotKilled = new HashSet<>(unusedSegments);
- segmentsToBeKilled.forEach(segmentsNotKilled::remove);
-
+ segmentsToKillFromDeepStore.forEach(segmentsNotKilled::remove);
if (!segmentsNotKilled.isEmpty()) {
LOG.warn(
- "Skipping kill of [%d] segments from deep storage as their load specs are used by other segments.",
- segmentsNotKilled.size()
+ "Skipping kill of [%d] segments of datasource[%s] from deep storage"
+ + " as their load specs are shared by other segments.",
+ segmentsNotKilled.size(), getDataSource()
);
}
- toolbox.getDataSegmentKiller().kill(segmentsToBeKilled);
- emitMetric(toolbox.getEmitter(), TaskMetrics.SEGMENTS_DELETED_FROM_DEEPSTORE, segmentsToBeKilled.size());
+ // 3. Nuke all eligible unused segments
Review Comment:
[P1] Close the sharing race before deleting deep storage
The new relationship lookup is only a preflight before `SegmentNukeAction`. With concurrent locks enabled this task holds a REPLACE lock, which can coexist with APPEND publication; an append publish or retry can therefore insert a parent or upgraded sibling sharing the load spec after step 2. Step 3 then removes the old metadata and step 4 deletes a file still referenced by the newly published segment. Keep the preflight, but perform a fail-closed sharing recheck after the nuke and before deep-storage deletion (ideally in a transaction/critical section that excludes a new publication window).
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -411,10 +446,11 @@ private List<DataSegment> getKillableSegments(
);
if (response != null && response.getUpgradedToSegmentIds() != null) {
response.getUpgradedToSegmentIds().forEach((parent, children) -> {
- if (!CollectionUtils.isNullOrEmpty(children)) {
- // Do not kill segment if its parent or any of its siblings still exist in metadata store
+ if (!segmentIdsBeingKilled.containsAll(children)) {
Review Comment:
[P1] Fail closed when the relationship response is null
If the action response itself or its `upgradedToSegmentIds` map is null, this guard skips the only new safety check and treats every candidate as unshared. Both the response DTO and remote conversion permit null, so a malformed or missing result can still proceed through metadata and deep-storage deletion. Throw before `SegmentNukeAction` unless a non-null map was returned.
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3644060349
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -251,62 +258,50 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
);
}
- // Kill segments. Order is important here:
- // Retrieve the segment upgrade infos for the batch _before_ the segments are nuked
- // We then want the nuke action to clean up the metadata records _before_ the segments are removed from storage.
- // This helps maintain that we will always have a storage segment if the metadata segment is present.
- // Determine the subset of segments to be killed from deep storage based on loadspecs.
- // If the segment nuke throws an exception, then the segment cleanup is abandoned.
-
- // Determine upgraded segment ids before nuking
- final Set<String> segmentIds = unusedSegments.stream()
- .map(DataSegment::getId)
- .map(SegmentId::toString)
- .collect(Collectors.toSet());
- final Map<String, String> upgradedFromSegmentIds = new HashMap<>();
- try {
- upgradedFromSegmentIds.putAll(
- taskActionClient.submit(
- new RetrieveUpgradedFromSegmentIdsAction(getDataSource(), segmentIds)
- ).getUpgradedFromSegmentIds()
- );
- }
- catch (Exception e) {
- LOG.warn(
- e,
- "Could not retrieve parent segment ids using task action[retrieveUpgradedFromSegmentIds]."
- + " Overlord may be on an older version."
- );
- }
+ // Kill segments - order of steps 1, 2, 3, 4 must remain the same
- // Nuke Segments
- taskActionClient.submit(new SegmentNukeAction(new HashSet<>(unusedSegments)));
- emitMetric(toolbox.getEmitter(), TaskMetrics.SEGMENTS_DELETED_FROM_METADATA_STORE, unusedSegments.size());
+ // 1. Determine parent segment ids of killable unused segments
+ final Map<String, String> upgradedFromSegmentIds
+ = fetchParentIdsForSegments(toolbox, unusedSegmentsPlus);
- // Determine segments to be killed
- final List<DataSegment> segmentsToBeKilled
- = getKillableSegments(unusedSegments, upgradedFromSegmentIds, usedSegmentLoadSpecs, taskActionClient);
+ // 2. Identify killable segments whose load specs are not shared with any other segment
+ final List<DataSegment> segmentsToKillFromDeepStore = getKillableSegments(
+ unusedSegments,
+ upgradedFromSegmentIds,
+ usedSegmentLoadSpecs,
+ taskActionClient
+ );
+ // 2a. Track segments that cannot be removed from deep store yet
final Set<DataSegment> segmentsNotKilled = new HashSet<>(unusedSegments);
- segmentsToBeKilled.forEach(segmentsNotKilled::remove);
-
+ segmentsToKillFromDeepStore.forEach(segmentsNotKilled::remove);
if (!segmentsNotKilled.isEmpty()) {
LOG.warn(
- "Skipping kill of [%d] segments from deep storage as their load specs are used by other segments.",
- segmentsNotKilled.size()
+ "Skipping kill of [%d] segments of datasource[%s] from deep storage"
Review Comment:
@abhishekrb19 , this is just a summary log. For each segment that is being skipped, there is a separate INFO log in the method `getKillableSegments`.
https://github.com/kfaraz/druid/blob/02612d9d2be7457d37fcaeaa707fe5ec9011b515/indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java#L452-L455
Please let me know if this suffices.
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #19737:
URL: https://github.com/apache/druid/pull/19737#issuecomment-5071860875
@abhishekrb19 , thanks a lot for the prompt review! I have updated the tests and added a couple of new ones.
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "abhishekrb19 (via GitHub)" <gi...@apache.org>.
abhishekrb19 commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3643738051
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -251,62 +258,50 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
);
}
- // Kill segments. Order is important here:
- // Retrieve the segment upgrade infos for the batch _before_ the segments are nuked
- // We then want the nuke action to clean up the metadata records _before_ the segments are removed from storage.
- // This helps maintain that we will always have a storage segment if the metadata segment is present.
- // Determine the subset of segments to be killed from deep storage based on loadspecs.
- // If the segment nuke throws an exception, then the segment cleanup is abandoned.
-
- // Determine upgraded segment ids before nuking
- final Set<String> segmentIds = unusedSegments.stream()
- .map(DataSegment::getId)
- .map(SegmentId::toString)
- .collect(Collectors.toSet());
- final Map<String, String> upgradedFromSegmentIds = new HashMap<>();
- try {
- upgradedFromSegmentIds.putAll(
- taskActionClient.submit(
- new RetrieveUpgradedFromSegmentIdsAction(getDataSource(), segmentIds)
- ).getUpgradedFromSegmentIds()
- );
- }
- catch (Exception e) {
- LOG.warn(
- e,
- "Could not retrieve parent segment ids using task action[retrieveUpgradedFromSegmentIds]."
- + " Overlord may be on an older version."
- );
- }
+ // Kill segments - order of steps 1, 2, 3, 4 must remain the same
- // Nuke Segments
- taskActionClient.submit(new SegmentNukeAction(new HashSet<>(unusedSegments)));
- emitMetric(toolbox.getEmitter(), TaskMetrics.SEGMENTS_DELETED_FROM_METADATA_STORE, unusedSegments.size());
+ // 1. Determine parent segment ids of killable unused segments
+ final Map<String, String> upgradedFromSegmentIds
+ = fetchParentIdsForSegments(toolbox, unusedSegmentsPlus);
- // Determine segments to be killed
- final List<DataSegment> segmentsToBeKilled
- = getKillableSegments(unusedSegments, upgradedFromSegmentIds, usedSegmentLoadSpecs, taskActionClient);
+ // 2. Identify killable segments whose load specs are not shared with any other segment
+ final List<DataSegment> segmentsToKillFromDeepStore = getKillableSegments(
+ unusedSegments,
+ upgradedFromSegmentIds,
+ usedSegmentLoadSpecs,
+ taskActionClient
+ );
+ // 2a. Track segments that cannot be removed from deep store yet
final Set<DataSegment> segmentsNotKilled = new HashSet<>(unusedSegments);
- segmentsToBeKilled.forEach(segmentsNotKilled::remove);
-
+ segmentsToKillFromDeepStore.forEach(segmentsNotKilled::remove);
if (!segmentsNotKilled.isEmpty()) {
LOG.warn(
- "Skipping kill of [%d] segments from deep storage as their load specs are used by other segments.",
- segmentsNotKilled.size()
+ "Skipping kill of [%d] segments of datasource[%s] from deep storage"
Review Comment:
Would it help to log the first N segment IDs or just a small list of intervals?
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -251,62 +258,50 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
);
}
- // Kill segments. Order is important here:
- // Retrieve the segment upgrade infos for the batch _before_ the segments are nuked
- // We then want the nuke action to clean up the metadata records _before_ the segments are removed from storage.
- // This helps maintain that we will always have a storage segment if the metadata segment is present.
- // Determine the subset of segments to be killed from deep storage based on loadspecs.
- // If the segment nuke throws an exception, then the segment cleanup is abandoned.
-
- // Determine upgraded segment ids before nuking
- final Set<String> segmentIds = unusedSegments.stream()
- .map(DataSegment::getId)
- .map(SegmentId::toString)
- .collect(Collectors.toSet());
- final Map<String, String> upgradedFromSegmentIds = new HashMap<>();
- try {
- upgradedFromSegmentIds.putAll(
- taskActionClient.submit(
- new RetrieveUpgradedFromSegmentIdsAction(getDataSource(), segmentIds)
- ).getUpgradedFromSegmentIds()
- );
- }
- catch (Exception e) {
- LOG.warn(
- e,
- "Could not retrieve parent segment ids using task action[retrieveUpgradedFromSegmentIds]."
- + " Overlord may be on an older version."
- );
- }
+ // Kill segments - order of steps 1, 2, 3, 4 must remain the same
- // Nuke Segments
- taskActionClient.submit(new SegmentNukeAction(new HashSet<>(unusedSegments)));
- emitMetric(toolbox.getEmitter(), TaskMetrics.SEGMENTS_DELETED_FROM_METADATA_STORE, unusedSegments.size());
+ // 1. Determine parent segment ids of killable unused segments
+ final Map<String, String> upgradedFromSegmentIds
+ = fetchParentIdsForSegments(toolbox, unusedSegmentsPlus);
- // Determine segments to be killed
- final List<DataSegment> segmentsToBeKilled
- = getKillableSegments(unusedSegments, upgradedFromSegmentIds, usedSegmentLoadSpecs, taskActionClient);
+ // 2. Identify killable segments whose load specs are not shared with any other segment
+ final List<DataSegment> segmentsToKillFromDeepStore = getKillableSegments(
+ unusedSegments,
+ upgradedFromSegmentIds,
+ usedSegmentLoadSpecs,
+ taskActionClient
+ );
+ // 2a. Track segments that cannot be removed from deep store yet
final Set<DataSegment> segmentsNotKilled = new HashSet<>(unusedSegments);
- segmentsToBeKilled.forEach(segmentsNotKilled::remove);
-
+ segmentsToKillFromDeepStore.forEach(segmentsNotKilled::remove);
if (!segmentsNotKilled.isEmpty()) {
LOG.warn(
- "Skipping kill of [%d] segments from deep storage as their load specs are used by other segments.",
- segmentsNotKilled.size()
+ "Skipping kill of [%d] segments of datasource[%s] from deep storage"
+ + " as their load specs are shared by other segments.",
Review Comment:
I think instead of "other", it'd help to provide the summary of the upgraded segment set
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -352,7 +347,47 @@ protected List<DataSegment> fetchNextBatchOfUnusedSegments(TaskToolbox toolbox,
nextBatchSize,
maxUsedStatusLastUpdatedTime
)
- );
+ )
+ .stream()
+ .map(segment -> new DataSegmentPlus(segment, null, null, null, null, null, null, null))
+ .collect(Collectors.toList());
+ }
+
+ /**
+ * Fetches the parent IDs (if any) for the given unused segments.
+ *
+ * @param unusedSegments Unused segments whose parent IDs need to be fetched
+ * @return Map from segment ID to the segment ID from which
+ * it was upgraded. If an input segment was not upgraded from any other segment,
+ * it does not have an entry in the map.
+ */
+ protected Map<String, String> fetchParentIdsForSegments(
+ TaskToolbox toolbox,
+ List<DataSegmentPlus> unusedSegments
+ )
+ {
+ try {
+ final Set<String> segmentIds = unusedSegments.stream().map(
+ s -> s.getDataSegment().getId().toString()
+ ).collect(Collectors.toSet());
+
+ return toolbox.getTaskActionClient().submit(
+ new RetrieveUpgradedFromSegmentIdsAction(getDataSource(), segmentIds)
+ ).getUpgradedFromSegmentIds();
+ }
+ catch (Exception e) {
+ // Do not proceed with killing these segments as we cannot be sure if their
+ // load spec is shared by any other segment or not. If load spec is shared,
+ // segment files cannot be deleted from deep store. If load spec is not
+ // shared, segments cannot be deleted from metadata store as that would
+ // leave deep store files orphaned, and they would never be cleaned up.
+ throw new ISE(
+ e,
+ "Could not retrieve parent segment ids using task action[retrieveUpgradedFromSegmentIds]."
+ + " Stopping kill task to avoid data loss in case the segment files"
+ + " are shared by other segments."
Review Comment:
What do you think about adding a unit test in `KillUnusedSegmentsTaskTest` or `UnusedSegmentsKillerTest` to verify that segments aren't spuriously purged in this case?
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3644580678
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -352,7 +347,47 @@ protected List<DataSegment> fetchNextBatchOfUnusedSegments(TaskToolbox toolbox,
nextBatchSize,
maxUsedStatusLastUpdatedTime
)
- );
+ )
+ .stream()
+ .map(segment -> new DataSegmentPlus(segment, null, null, null, null, null, null, null))
+ .collect(Collectors.toList());
+ }
+
+ /**
+ * Fetches the parent IDs (if any) for the given unused segments.
+ *
+ * @param unusedSegments Unused segments whose parent IDs need to be fetched
+ * @return Map from segment ID to the segment ID from which
+ * it was upgraded. If an input segment was not upgraded from any other segment,
+ * it does not have an entry in the map.
+ */
+ protected Map<String, String> fetchParentIdsForSegments(
+ TaskToolbox toolbox,
+ List<DataSegmentPlus> unusedSegments
+ )
+ {
+ try {
+ final Set<String> segmentIds = unusedSegments.stream().map(
+ s -> s.getDataSegment().getId().toString()
+ ).collect(Collectors.toSet());
+
+ return toolbox.getTaskActionClient().submit(
+ new RetrieveUpgradedFromSegmentIdsAction(getDataSource(), segmentIds)
+ ).getUpgradedFromSegmentIds();
+ }
+ catch (Exception e) {
+ // Do not proceed with killing these segments as we cannot be sure if their
+ // load spec is shared by any other segment or not. If load spec is shared,
+ // segment files cannot be deleted from deep store. If load spec is not
+ // shared, segments cannot be deleted from metadata store as that would
+ // leave deep store files orphaned, and they would never be cleaned up.
+ throw new ISE(
+ e,
+ "Could not retrieve parent segment ids using task action[retrieveUpgradedFromSegmentIds]."
+ + " Stopping kill task to avoid data loss in case the segment files"
+ + " are shared by other segments."
Review Comment:
Thanks for the suggestion! Added some tests in `KillUnusedSegmentsTaskTest`.
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3646522852
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -251,62 +258,46 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
);
}
- // Kill segments. Order is important here:
- // Retrieve the segment upgrade infos for the batch _before_ the segments are nuked
- // We then want the nuke action to clean up the metadata records _before_ the segments are removed from storage.
- // This helps maintain that we will always have a storage segment if the metadata segment is present.
- // Determine the subset of segments to be killed from deep storage based on loadspecs.
- // If the segment nuke throws an exception, then the segment cleanup is abandoned.
-
- // Determine upgraded segment ids before nuking
- final Set<String> segmentIds = unusedSegments.stream()
- .map(DataSegment::getId)
- .map(SegmentId::toString)
- .collect(Collectors.toSet());
- final Map<String, String> upgradedFromSegmentIds = new HashMap<>();
- try {
- upgradedFromSegmentIds.putAll(
- taskActionClient.submit(
- new RetrieveUpgradedFromSegmentIdsAction(getDataSource(), segmentIds)
- ).getUpgradedFromSegmentIds()
- );
- }
- catch (Exception e) {
- LOG.warn(
- e,
- "Could not retrieve parent segment ids using task action[retrieveUpgradedFromSegmentIds]."
- + " Overlord may be on an older version."
- );
- }
+ // Kill segments - order of steps 1, 2, 3, 4 must remain the same
- // Nuke Segments
- taskActionClient.submit(new SegmentNukeAction(new HashSet<>(unusedSegments)));
- emitMetric(toolbox.getEmitter(), TaskMetrics.SEGMENTS_DELETED_FROM_METADATA_STORE, unusedSegments.size());
+ // 1. Determine parent segment ids of killable unused segments
+ final Map<String, String> upgradedFromSegmentIds
+ = fetchParentIdsForSegments(toolbox, unusedSegmentsPlus);
- // Determine segments to be killed
- final List<DataSegment> segmentsToBeKilled
- = getKillableSegments(unusedSegments, upgradedFromSegmentIds, usedSegmentLoadSpecs, taskActionClient);
+ // 2. Identify killable segments whose load specs are not shared with any other segment
+ final List<DataSegment> segmentsToKillFromDeepStore = getKillableSegments(
+ unusedSegments,
+ upgradedFromSegmentIds,
+ usedSegmentLoadSpecs,
+ taskActionClient
+ );
+ // 2a. Track segments that cannot be removed from deep store yet
final Set<DataSegment> segmentsNotKilled = new HashSet<>(unusedSegments);
- segmentsToBeKilled.forEach(segmentsNotKilled::remove);
-
+ segmentsToKillFromDeepStore.forEach(segmentsNotKilled::remove);
if (!segmentsNotKilled.isEmpty()) {
LOG.warn(
- "Skipping kill of [%d] segments from deep storage as their load specs are used by other segments.",
- segmentsNotKilled.size()
+ "Skipping kill of [%d] segments of datasource[%s] from deep storage"
+ + " as their load specs are shared by other segments.",
+ segmentsNotKilled.size(), getDataSource()
);
}
- toolbox.getDataSegmentKiller().kill(segmentsToBeKilled);
- emitMetric(toolbox.getEmitter(), TaskMetrics.SEGMENTS_DELETED_FROM_DEEPSTORE, segmentsToBeKilled.size());
+ // 3. Nuke all eligible unused segments
Review Comment:
It is very unlikely for this to happen. It had been called out here as well.
https://github.com/apache/druid/pull/16362#discussion_r1594909953
Essentially, the kill buffer period protects us against this.
Concurrent APPEND would only perform upgrades on a "used" segment.
And kill task would only nuke segments that have been been "unused" for a while (default is 30 days).
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz merged PR #19737:
URL: https://github.com/apache/druid/pull/19737
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3650025234
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -236,12 +238,20 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
break;
}
- unusedSegments = fetchNextBatchOfUnusedSegments(toolbox, nextBatchSize);
+ unusedSegmentsPlus = fetchNextBatchOfUnusedSegments(toolbox, nextBatchSize);
+ if (unusedSegmentsPlus.isEmpty()) {
+ // No more segments eligible for kill, do not proceed further
+ break;
+ }
// Fetch locks each time as a revokal could have occurred in between batches
final NavigableMap<DateTime, List<TaskLock>> taskLockMap
= getNonRevokedTaskLockMap(toolbox.getTaskActionClient());
+ final Set<DataSegment> unusedSegments = unusedSegmentsPlus.stream()
+ .map(DataSegmentPlus::getDataSegment)
+ .collect(Collectors.toSet());
+
if (!TaskLocks.isLockCoversSegments(taskLockMap, unusedSegments)) {
Review Comment:
The `Set<DataSegment> unusedSegments` created before this line is also used later in this method.
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3649595662
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -87,6 +85,10 @@
* <li> Filter the set of unreferenced segments using load specs from the set of used segments. </li>
* <li> Kill the filtered set of segments from deep storage. </li>
* </ol>
+ * Note: When {@link Tasks#USE_CONCURRENT_LOCKS} is true, keep a large buffer
+ * period before killing segments after they have been marked as unused.
+ * Otherwise, there may be a potential data loss if a concurrent APPEND job
Review Comment:
Yeah, good point, I was thinking the same.
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3644062378
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -251,62 +258,50 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
);
}
- // Kill segments. Order is important here:
- // Retrieve the segment upgrade infos for the batch _before_ the segments are nuked
- // We then want the nuke action to clean up the metadata records _before_ the segments are removed from storage.
- // This helps maintain that we will always have a storage segment if the metadata segment is present.
- // Determine the subset of segments to be killed from deep storage based on loadspecs.
- // If the segment nuke throws an exception, then the segment cleanup is abandoned.
-
- // Determine upgraded segment ids before nuking
- final Set<String> segmentIds = unusedSegments.stream()
- .map(DataSegment::getId)
- .map(SegmentId::toString)
- .collect(Collectors.toSet());
- final Map<String, String> upgradedFromSegmentIds = new HashMap<>();
- try {
- upgradedFromSegmentIds.putAll(
- taskActionClient.submit(
- new RetrieveUpgradedFromSegmentIdsAction(getDataSource(), segmentIds)
- ).getUpgradedFromSegmentIds()
- );
- }
- catch (Exception e) {
- LOG.warn(
- e,
- "Could not retrieve parent segment ids using task action[retrieveUpgradedFromSegmentIds]."
- + " Overlord may be on an older version."
- );
- }
+ // Kill segments - order of steps 1, 2, 3, 4 must remain the same
- // Nuke Segments
- taskActionClient.submit(new SegmentNukeAction(new HashSet<>(unusedSegments)));
- emitMetric(toolbox.getEmitter(), TaskMetrics.SEGMENTS_DELETED_FROM_METADATA_STORE, unusedSegments.size());
+ // 1. Determine parent segment ids of killable unused segments
+ final Map<String, String> upgradedFromSegmentIds
+ = fetchParentIdsForSegments(toolbox, unusedSegmentsPlus);
- // Determine segments to be killed
- final List<DataSegment> segmentsToBeKilled
- = getKillableSegments(unusedSegments, upgradedFromSegmentIds, usedSegmentLoadSpecs, taskActionClient);
+ // 2. Identify killable segments whose load specs are not shared with any other segment
+ final List<DataSegment> segmentsToKillFromDeepStore = getKillableSegments(
+ unusedSegments,
+ upgradedFromSegmentIds,
+ usedSegmentLoadSpecs,
+ taskActionClient
+ );
+ // 2a. Track segments that cannot be removed from deep store yet
final Set<DataSegment> segmentsNotKilled = new HashSet<>(unusedSegments);
- segmentsToBeKilled.forEach(segmentsNotKilled::remove);
-
+ segmentsToKillFromDeepStore.forEach(segmentsNotKilled::remove);
if (!segmentsNotKilled.isEmpty()) {
LOG.warn(
- "Skipping kill of [%d] segments from deep storage as their load specs are used by other segments.",
- segmentsNotKilled.size()
+ "Skipping kill of [%d] segments of datasource[%s] from deep storage"
+ + " as their load specs are shared by other segments.",
Review Comment:
Please refer to https://github.com/apache/druid/pull/19737/#discussion_r3644060349
--
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] fix: Stop kill task if segment IDs that share load spec of killable segment could not be determined (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19737:
URL: https://github.com/apache/druid/pull/19737#discussion_r3650025234
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/KillUnusedSegmentsTask.java:
##########
@@ -236,12 +238,20 @@ public TaskStatus runTask(TaskToolbox toolbox) throws Exception
break;
}
- unusedSegments = fetchNextBatchOfUnusedSegments(toolbox, nextBatchSize);
+ unusedSegmentsPlus = fetchNextBatchOfUnusedSegments(toolbox, nextBatchSize);
+ if (unusedSegmentsPlus.isEmpty()) {
+ // No more segments eligible for kill, do not proceed further
+ break;
+ }
// Fetch locks each time as a revokal could have occurred in between batches
final NavigableMap<DateTime, List<TaskLock>> taskLockMap
= getNonRevokedTaskLockMap(toolbox.getTaskActionClient());
+ final Set<DataSegment> unusedSegments = unusedSegmentsPlus.stream()
+ .map(DataSegmentPlus::getDataSegment)
+ .collect(Collectors.toSet());
+
if (!TaskLocks.isLockCoversSegments(taskLockMap, unusedSegments)) {
Review Comment:
The Set `unusedSegments` created above is also used later in this method.
--
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