fix #1715 #1740

nishantmonu51 · 2015-09-16T20:36:00Z

TaskLockBox has a set of active tasks
lock requests throws exception for if they are from a task not in
active task set.
TaskQueue is responsible for updating the active task set on
tasklockbox

drcrallen · 2015-09-16T20:56:30Z

indexing-service/src/main/java/io/druid/indexing/overlord/TaskLockbox.java

   */
  private Optional<TaskLock> tryLock(final Task task, final Interval interval, final Optional<String> preferredVersion)
  {
    giant.lock();

    try {
+      if(!tasks.contains(task)){


This has me a bit worried because then every task needs to implement equals and hash properly, which I'm not sure was enforced prior to this. Is there a reason it cannot be a set of the string TaskID?

makes sense, will change it to store taskId.

drcrallen · 2015-09-16T21:08:05Z

indexing-service/src/main/java/io/druid/indexing/overlord/TaskLockbox.java

@@ -66,6 +66,9 @@

  private static final EmittingLogger log = new EmittingLogger(TaskLockbox.class);

+  // Stores List of Active Tasks. TaskLockbox will only grant locks to active activeTasks.
+  private final Set<String> activeTasks = Sets.newHashSet();


Does this need memory protection of the big lock to access or modify? If so it is probably worth calling out here.

drcrallen · 2015-09-16T21:14:38Z

indexing-service/src/main/java/io/druid/indexing/overlord/TaskLockbox.java

   */
  private Optional<TaskLock> tryLock(final Task task, final Interval interval, final Optional<String> preferredVersion)
  {
    giant.lock();

    try {
+      if(!activeTasks.contains(task.getId())){


Why throw ISE instead of returning absent?

modified to return absent for invalid tasks.

If it doesn't throw something then "lock" will spin forever.

gianm · 2015-09-18T01:01:07Z

indexing-service/src/main/java/io/druid/indexing/overlord/TaskLockbox.java

+  {
+    giant.lock();
+    try {
+      return activeTasks.remove(task.getId());


I think this method should be combined with unlock(Task), they're both called at the same time (by the queue, when a task finishes).

This method should also be signalling the lockReleaseCondition, since when a task is removed then we want to ISE out of any lock(task) calls for that task.

nishantmonu51 · 2015-09-18T15:42:50Z

@gianm have taken care of your comments. please have a look again

drcrallen · 2015-09-22T17:21:57Z

indexing-service/src/main/java/io/druid/indexing/overlord/TaskLockbox.java

    }
    finally {
+      activeTasks.remove(task.getId());


unlock should have a finally block of its own, even if this remove is in its own try{}finally{} block.

fjy · 2015-09-23T23:55:50Z

indexing-service/src/main/java/io/druid/indexing/overlord/TaskLockbox.java

   */
  private Optional<TaskLock> tryLock(final Task task, final Interval interval, final Optional<String> preferredVersion)
  {
    giant.lock();

    try {
+      if(!activeTasks.contains(task.getId())){
+        throw new IllegalStateException(String.format("Unable to grant lock to inactive Task [%s]", task.getId()));


small comment, but why don't we use ISE here instead?

fjy · 2015-09-23T23:58:57Z

👍

fixes apache#1715 - TaskLockBox has a set of active tasks - lock requests throws exception for if they are from a task not in active task set. - TaskQueue is responsible for updating the active task set on tasklockbox fix apache#1715 fixes apache#1715 - TaskLockBox has a set of active tasks - lock requests throws exception for if they are from a task not in active task set. - TaskQueue is responsible for updating the active task set on tasklockbox review comment remove duplicate line use ISE instead organise imports

fix #1715

drcrallen reviewed Sep 16, 2015
View reviewed changes

nishantmonu51 force-pushed the validate-locks branch from 3af08b1 to 20c8975 Compare September 16, 2015 21:03

drcrallen reviewed Sep 16, 2015
View reviewed changes

nishantmonu51 force-pushed the validate-locks branch from 20c8975 to 5f8cd37 Compare September 16, 2015 21:13

drcrallen reviewed Sep 16, 2015
View reviewed changes

nishantmonu51 force-pushed the validate-locks branch 3 times, most recently from 5c21b80 to c0da8dc Compare September 17, 2015 11:24

gianm reviewed Sep 18, 2015
View reviewed changes

nishantmonu51 force-pushed the validate-locks branch from c0da8dc to face055 Compare September 18, 2015 11:42

drcrallen added this to the 0.8.2 milestone Sep 22, 2015

drcrallen reviewed Sep 22, 2015
View reviewed changes

nishantmonu51 force-pushed the validate-locks branch from face055 to 4f0e65c Compare September 22, 2015 17:55

fjy reviewed Sep 23, 2015
View reviewed changes

nishantmonu51 force-pushed the validate-locks branch from 4f0e65c to 4da45a0 Compare September 24, 2015 04:19

nishantmonu51 force-pushed the validate-locks branch from 4da45a0 to b638400 Compare September 24, 2015 04:37

drcrallen added a commit that referenced this pull request Sep 29, 2015

Merge pull request #1740 from metamx/validate-locks

d2e400f

fix #1715

drcrallen merged commit d2e400f into apache:master Sep 29, 2015

drcrallen deleted the validate-locks branch September 29, 2015 16:38

gianm mentioned this pull request Nov 2, 2015

Priority based task locking #1679

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #1715 #1740

fix #1715 #1740

nishantmonu51 commented Sep 16, 2015

drcrallen Sep 16, 2015

nishantmonu51 Sep 16, 2015

nishantmonu51 Sep 16, 2015

drcrallen Sep 16, 2015

drcrallen Sep 16, 2015

nishantmonu51 Sep 17, 2015

gianm Sep 18, 2015

nishantmonu51 Sep 22, 2015

gianm Sep 18, 2015

nishantmonu51 commented Sep 18, 2015

drcrallen Sep 22, 2015

nishantmonu51 Sep 22, 2015

fjy Sep 23, 2015

fjy commented Sep 23, 2015

fix #1715 #1740

fix #1715 #1740

Conversation

nishantmonu51 commented Sep 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishantmonu51 commented Sep 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjy commented Sep 23, 2015