kfaraz commented on code in PR #19772: URL: https://github.com/apache/druid/pull/19772#discussion_r3685242590
########## indexing-service/src/main/java/org/apache/druid/indexing/overlord/duty/UnusedSegmentsKiller.java: ########## @@ -75,25 +84,41 @@ public class UnusedSegmentsKiller implements OverlordDuty private static final String TASK_ID_PREFIX = "overlord-issued"; + /** + * Use concurrent locks by default. Review Comment: The older comment added in #19737 was somewhat incorrect. The only case where a kill task could cause a potential data loss was if something like this happened: - kill task starts and identifies the unused segments to kill - markAsUsed API marks some of the target unused segments as used - a concurrent APPEND job upgrades some of these segments - kill task still thinks that the segments are unused and kill their metadata entry as well as deep storage files This patch originally included a javadoc comment explaining the above. But then I decided that we might as well address the underlying issue, i.e. make the `markAsUsed` API mutually exclusive with any indexing/kill task. The docs already mention that calling these APIs while a task is in progress may lead to erratic behaviour. Please let me know what you think. -- 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]
