[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #3092: [GOBBLIN-1251] Propagate exception in TaskStateTracker for caller to trigger Helix retry

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

sv2000 commented on a change in pull request #3092:
URL: https://github.com/apache/incubator-gobblin/pull/3092#discussion_r477621504



##########
File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTaskStateTracker.java
##########
@@ -59,7 +58,11 @@ public void registerNewTask(Task task) {
     try {
       this.scheduledReporters.put(task.getTaskId(), scheduleTaskMetricsUpdater(new TaskMetricsUpdater(task), task));
     } catch (RejectedExecutionException ree) {
+      // Propagate the exception to caller that has full control of the life-cycle of a helix task.
       LOGGER.error(String.format("Scheduling of task state reporter for task %s was rejected", task.getTaskId()));
+      Throwables.propagate(ree);

Review comment:
       One question worth considering: Do we want to make the failure in scheduling TaskMetricsUpdater a fatal failure? or can it be made configurable? 

##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/GobblinMultiTaskAttempt.java
##########
@@ -426,7 +431,7 @@ private boolean taskSuccessfulInPriorAttempt(String taskId) {
         if (task == null) {
           if (e instanceof RetryException) {
             // Indicating task being null due to failure in creation even after retrying.
-            isTaskCreatedSuccessfully = false;
+            areAllTaskSubmitted = false;

Review comment:
       areAllTaskSubmitted -> areAllTasksSubmitted.

##########
File path: gobblin-runtime/src/main/java/org/apache/gobblin/runtime/GobblinMultiTaskAttempt.java
##########
@@ -385,7 +387,10 @@ private boolean taskSuccessfulInPriorAttempt(String taskId) {
   private Pair<List<Task>, Boolean> runWorkUnits(CountUpAndDownLatch countDownLatch) {
 
     List<Task> tasks = Lists.newArrayList();
-    boolean isTaskCreatedSuccessfully = true;
+
+    // A flag indicating if there are any tasks not submitted successfully.
+    // Caller of this method should handler un-submitted task accordingly.

Review comment:
       handler -> handle.
   
   un-submitted task -> tasks with submission failures




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