capistrant commented on code in PR #19921:
URL: https://github.com/apache/druid/pull/19921#discussion_r3750632829
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java:
##########
@@ -997,20 +1016,53 @@ public int markNonOvershadowedSegmentsAsUsed(
return markNonOvershadowedSegmentsAsUsed(unusedSegments, timeline,
updateTime);
}
- private List<DataSegment> retrieveUnusedSegments(
+ /**
+ * Checks that all the segments were last updated within the kill buffer
period.
+ * If any segment was updated earlier than that, an exception is thrown so
that
+ * none of the segments are updated to ensure atomicity.
+ */
+ private void validateSegmentsForMarkingAsUsed(Collection<DataSegmentPlus>
segments)
+ {
+ if (!managerConfig.getKillUnused().isEnabled()) {
+ // Do not verify the buffer period if embedded kill tasks are not enabled
+ return;
+ }
+
+ final Period bufferPeriod =
managerConfig.getKillUnused().getBufferPeriod();
+ final DateTime minAllowedUpdateTime = DateTimes.nowUtc().minus(
+ managerConfig.getKillUnused().getBufferPeriod()
+ );
+
+ final List<SegmentId> expiredSegmentIds = segments.stream().filter(
+ s -> s.getUsedStatusLastUpdatedDate() != null
+ && !s.getUsedStatusLastUpdatedDate().isAfter(minAllowedUpdateTime)
+ ).map(s -> s.getDataSegment().getId()).toList();
+
+ if (!expiredSegmentIds.isEmpty()) {
+ throw DruidException.forPersona(DruidException.Persona.OPERATOR)
+ .ofCategory(DruidException.Category.CONFLICT)
+ .build(
+ "Segment IDs[%s] cannot be marked as used since"
+ + " they were last updated more than [%s] ago
and"
+ + " are now eligible for permanent deletion.",
+ expiredSegmentIds, bufferPeriod
+ );
Review Comment:
does this new restriction need to be in docs? perhaps in the properties
table that describes what the buffer period is and what happens after the
segment ages out of the buffer?
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java:
##########
@@ -997,20 +1016,53 @@ public int markNonOvershadowedSegmentsAsUsed(
return markNonOvershadowedSegmentsAsUsed(unusedSegments, timeline,
updateTime);
}
- private List<DataSegment> retrieveUnusedSegments(
+ /**
+ * Checks that all the segments were last updated within the kill buffer
period.
+ * If any segment was updated earlier than that, an exception is thrown so
that
+ * none of the segments are updated to ensure atomicity.
+ */
+ private void validateSegmentsForMarkingAsUsed(Collection<DataSegmentPlus>
segments)
+ {
+ if (!managerConfig.getKillUnused().isEnabled()) {
+ // Do not verify the buffer period if embedded kill tasks are not enabled
+ return;
+ }
+
+ final Period bufferPeriod =
managerConfig.getKillUnused().getBufferPeriod();
+ final DateTime minAllowedUpdateTime = DateTimes.nowUtc().minus(
+ managerConfig.getKillUnused().getBufferPeriod()
+ );
+
+ final List<SegmentId> expiredSegmentIds = segments.stream().filter(
+ s -> s.getUsedStatusLastUpdatedDate() != null
+ && !s.getUsedStatusLastUpdatedDate().isAfter(minAllowedUpdateTime)
+ ).map(s -> s.getDataSegment().getId()).toList();
+
+ if (!expiredSegmentIds.isEmpty()) {
+ throw DruidException.forPersona(DruidException.Persona.OPERATOR)
+ .ofCategory(DruidException.Category.CONFLICT)
+ .build(
+ "Segment IDs[%s] cannot be marked as used since"
+ + " they were last updated more than [%s] ago
and"
+ + " are now eligible for permanent deletion.",
+ expiredSegmentIds, bufferPeriod
+ );
Review Comment:
documenting depends a lot on the outcome of my comment above related to only
enforcing this when using embedded kill
##########
server/src/main/java/org/apache/druid/metadata/segment/SqlSegmentMetadataTransactionFactory.java:
##########
@@ -142,6 +147,16 @@ public <T> T inReadWriteDatasourceTransaction(
);
}
+ @Override
+ public <T> T
inReadWriteNoCacheTransaction(Function<SqlSegmentsMetadataQuery, T> sqlUpdate)
+ {
+ return connector.retryReadOnlyTransaction(
Review Comment:
should this be calling `connector.retryTransaction`?
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskLockbox.java:
##########
@@ -571,6 +572,13 @@ private TaskLockPosse createOrFindLockPosse(LockRequest
request, Task task, bool
throw new ISE("Unable to grant LockPosse to inactive Task [%s]",
task.getId());
}
+ if (request.getType() == TaskLockType.KILL &&
!KillUnusedSegmentsTask.TYPE.equals(task.getType())) {
Review Comment:
so manually submitted kill tasks can use the kill lock type, as far as I can
tell at least. is that acceptable?
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java:
##########
@@ -997,20 +1016,53 @@ public int markNonOvershadowedSegmentsAsUsed(
return markNonOvershadowedSegmentsAsUsed(unusedSegments, timeline,
updateTime);
}
- private List<DataSegment> retrieveUnusedSegments(
+ /**
+ * Checks that all the segments were last updated within the kill buffer
period.
+ * If any segment was updated earlier than that, an exception is thrown so
that
+ * none of the segments are updated to ensure atomicity.
+ */
+ private void validateSegmentsForMarkingAsUsed(Collection<DataSegmentPlus>
segments)
+ {
+ if (!managerConfig.getKillUnused().isEnabled()) {
+ // Do not verify the buffer period if embedded kill tasks are not enabled
+ return;
+ }
Review Comment:
hmmm. do we really need to introduce this exception to the rule? Having the
whole system opted into the new restriction about how long a segment can be
unused before not being eligible to mark used seems less sconfusing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
