clintropolis commented on a change in pull request #11788:
URL: https://github.com/apache/druid/pull/11788#discussion_r727638115
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/TimeChunkLockTryAcquireAction.java
##########
@@ -81,7 +81,8 @@ public TaskLock perform(Task task, TaskActionToolbox toolbox)
task,
new TimeChunkLockRequest(type, task, interval, null)
);
- return result.isOk() ? result.getTaskLock() : null;
+
+ return result.getTaskLock();
Review comment:
hmm, this doesn't match `TimeChunkLockAcquireAction` anymore, does it
also need updated to match this signature?
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/common/task/AbstractBatchIndexTask.java
##########
@@ -404,6 +405,9 @@ protected boolean tryTimeChunkLock(TaskActionClient client,
List<Interval> inter
if (lock == null) {
return false;
}
+ if (lock.isRevoked()) {
Review comment:
i wonder if there is some sort of analysis we could add to make build
complain at us if we are getting a task lock anywhere and not checking if it is
revoked... I haven't a very good idea on how to actually do this though, just a
bit worried that this could be error prone.
It seems a much bigger refactoring to just make the methods that acquire the
locks explode and throw some specialized 'lock revoked' exception, and also to
sort of violate the current contract of the 'try acquire' methods, so i'm not
really sure that is a good approach either.
--
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]
