[GitHub] [gobblin] autumnust commented on a change in pull request #3250: GOBBLIN-1416: Fix a race condition caused by Helix task cancellation being invoked before Gobblin task creation

[GitBox <gi...@apache.org>]

autumnust commented on a change in pull request #3250:
URL: https://github.com/apache/gobblin/pull/3250#discussion_r598020976



##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/GobblinMultiTaskAttempt.java
##########
@@ -384,10 +394,14 @@ private boolean taskSuccessfulInPriorAttempt(String taskId) {
    * @return a list of {@link Task}s from the {@link WorkUnit}s, as well as if there's a failure in task creation
    * which should be handled separately to avoid silently starving on certain workunit.
    */
-  private Pair<List<Task>, Boolean> runWorkUnits(CountUpAndDownLatch countDownLatch) {
-
+  private synchronized Pair<List<Task>, Boolean> runWorkUnits(CountUpAndDownLatch countDownLatch) {
     List<Task> tasks = Lists.newArrayList();
-
+    //Has the task-attempt already been cancelled? This can happen for instance when a cancellation has been invoked on
+    // the GobblinMultiTaskAttempt instance (e.g. in the case of Helix task cancellation) before the Gobblin tasks
+    // have been submitted to the underlying task executor.
+    if (this.stopped.get()) {

Review comment:
       It doesn't seem to be enough for synchronization purpose. cancel could happen after this line and between line 407 and 408. 




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org