You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2020/03/10 22:42:37 UTC

[GitHub] [incubator-gobblin] autumnust opened a new pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

autumnust opened a new pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

### JIRA
- https://issues.apache.org/jira/browse/GOBBLIN-1078

### Description
The task cancel and task.run is called in different thread from helix side, which introduced problems when:
- Task get cancelled for some reasons from Helix side, therefore calling `SingleTask#cancel `
- However, TaskAttempt object within SingleTask has not been initialized, therefore it didn't pass the non-null checking and only prints a log. After that, the run method in different thread start to run and never get cancelled.

This sequence could result in violation of helix quota in each participant: While helix believe there's no partition assigned to a participant and assign a new partition, the previous not-yet-cancelled partition is still running.

We added a barrier in cancel method to ensure cancel is blocked if taskAttempt object is not yet being initialized. Added unit test to simulate the sequence as well.

### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:

### Commits
- [ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

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

With regards,
Apache Git Services

[GitHub] [incubator-gobblin] ZihanLi58 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

ZihanLi58 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#discussion_r391159295
 
 

 ##########
 File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/SingleTask.java
 ##########
 @@ -95,7 +108,16 @@ public void run()
         .createDefaultTopLevelBroker(jobConfig, GobblinScopeTypes.GLOBAL.defaultScopeInstance())) {
       SharedResourcesBroker<GobblinScopeTypes> jobBroker = getJobBroker(_jobState, globalBroker);
 
-      _taskAttempt = _taskAttemptBuilder.build(workUnits.iterator(), _jobId, _jobState, jobBroker);
+      // Secure atomicity of taskAttempt's execution.
+      // Signaling blocking threads if any whenever taskAttempt is nonNull.
+      _lock.lock();
+      try {
+        _taskAttempt = _taskAttemptBuilder.build(getWorkUnits().iterator(), _jobId, _jobState, jobBroker);
+        _taskAttemptBuilt.signal();
+      } finally {
+        _lock.unlock();
+      }
+
       _taskAttempt.runAndOptionallyCommitTaskAttempt(GobblinMultiTaskAttempt.CommitPolicy.IMMEDIATE);
 
 Review comment:
   why do we move this out of the lock? What will happen if _taskAttemp is initiated but not run, and cancel happen and then call _taskAttempt.runAndOptionallyCommitTaskAttempt() 

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] codecov-io commented on issue #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

codecov-io commented on issue #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#issuecomment-598028158
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=h1) Report
   > Merging [#2919](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/7a328f9a232a60973d27c50859e6b84e63df90f7&el=desc) will **increase** coverage by `41.35%`.
   > The diff coverage is `78.94%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=tree)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #2919       +/-   ##
   =============================================
   + Coverage      4.12%   45.48%   +41.35%     
   - Complexity      750     9110     +8360     
   =============================================
     Files          1936     1936               
     Lines         73046    73058       +12     
     Branches       8046     8050        +4     
   =============================================
   + Hits           3016    33229    +30213     
   + Misses        69710    36772    -32938     
   - Partials        320     3057     +2737     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...a/org/apache/gobblin/cluster/GobblinHelixTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkhlbGl4VGFzay5qYXZh) | `77.61% <ø> (+77.61%)` | `6.00 <0.00> (+6.00)` | |
   | [...in/java/org/apache/gobblin/cluster/SingleTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2luZ2xlVGFzay5qYXZh) | `84.84% <78.94%> (+84.84%)` | `13.00 <5.00> (+13.00)` | |
   | [...gobblin/service/monitoring/JobStatusRetriever.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9Kb2JTdGF0dXNSZXRyaWV2ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `2.00% <0.00%> (+2.00%)` | |
   | [...ata/management/copy/hive/HivePartitionFileSet.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvaGl2ZS9IaXZlUGFydGl0aW9uRmlsZVNldC5qYXZh) | `0.95% <0.00%> (+0.95%)` | `1.00% <0.00%> (+1.00%)` | |
   | [...ain/java/org/apache/gobblin/runtime/TaskState.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvVGFza1N0YXRlLmphdmE=) | `81.97% <0.00%> (+1.16%)` | `32.00% <0.00%> (ø%)` | |
   | [...pache/gobblin/runtime/GobblinMultiTaskAttempt.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvR29iYmxpbk11bHRpVGFza0F0dGVtcHQuamF2YQ==) | `57.32% <0.00%> (+1.25%)` | `28.00% <0.00%> (+2.00%)` | |
   | [...ava/org/apache/gobblin/runtime/MultiConverter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvTXVsdGlDb252ZXJ0ZXIuamF2YQ==) | `83.60% <0.00%> (+1.63%)` | `9.00% <0.00%> (+1.00%)` | |
   | [...rg/apache/gobblin/runtime/FsDatasetStateStore.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvRnNEYXRhc2V0U3RhdGVTdG9yZS5qYXZh) | `73.80% <0.00%> (+1.78%)` | `35.00% <0.00%> (+1.00%)` | |
   | [.../java/org/apache/gobblin/runtime/TaskExecutor.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvVGFza0V4ZWN1dG9yLmphdmE=) | `45.35% <0.00%> (+2.73%)` | `9.00% <0.00%> (+1.00%)` | |
   | [...org/apache/gobblin/yarn/GobblinYarnTaskRunner.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbllhcm5UYXNrUnVubmVyLmphdmE=) | `2.98% <0.00%> (+2.98%)` | `1.00% <0.00%> (+1.00%)` | |
   | ... and [1112 more](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=footer). Last update [7a328f9...751e0a1](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

sv2000 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#discussion_r404294682
 
 

 ##########
 File path: gobblin-cluster/src/test/java/org/apache/gobblin/cluster/ClusterIntegrationTest.java
 ##########
 @@ -80,28 +83,49 @@ private HelixManager getHelixManager() {
 
   @Test
   void testJobShouldGetCancelled() throws Exception {
+    // Cancellation usually needs long time to successfully be executed, therefore seeing the sleeping time to 100.
 
 Review comment:
   Typo: seeing -> setting

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

sv2000 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#discussion_r394054690
 
 

 ##########
 File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/SingleTask.java
 ##########
 @@ -93,8 +109,17 @@ public void run()
         .createDefaultTopLevelBroker(jobConfig, GobblinScopeTypes.GLOBAL.defaultScopeInstance())) {
       SharedResourcesBroker<GobblinScopeTypes> jobBroker = getJobBroker(_jobState, globalBroker);
 
-      _taskAttempt = _taskAttemptBuilder.build(workUnits.iterator(), _jobId, _jobState, jobBroker);
-      _taskAttempt.runAndOptionallyCommitTaskAttempt(GobblinMultiTaskAttempt.CommitPolicy.IMMEDIATE);
+      // Secure atomicity of taskAttempt's execution.
+      // Signaling blocking threads if any whenever taskAttempt is nonNull.
+      _taskAttempt = _taskAttemptBuilder.build(getWorkUnits().iterator(), _jobId, _jobState, jobBroker);
+
+      _lock.lock();
+      try {
+        _taskAttemptBuilt.signal();
+        _taskAttempt.runAndOptionallyCommitTaskAttempt(GobblinMultiTaskAttempt.CommitPolicy.IMMEDIATE);
 
 Review comment:
   Maybe I am missing something obvious - won't this block until the taskAttempt is finished? If so, is it right to hold the lock() until the attempt is finished? For instance, if a cancel() is invoked, won't the cancel() be waiting forever for the lock()? Alternately, if the cancel() acquires the lock() before the taskAttempt is built, then taskAttempt may be created after the cancel() fails.

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

sv2000 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#discussion_r394049288
 
 

 ##########
 File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixTask.java
 ##########
 @@ -19,9 +19,14 @@
 
 import java.util.Map;
 import java.util.concurrent.Callable;
-import java.util.concurrent.TimeUnit;
 
-import org.apache.gobblin.util.ConfigUtils;
+import org.apache.gobblin.annotation.Alpha;
 
 Review comment:
   Is this class still Alpha? Maybe time to drop the @Alpha annotation.

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] ZihanLi58 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

ZihanLi58 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#discussion_r390667869
 
 

 ##########
 File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/SingleTask.java
 ##########
 @@ -63,6 +69,12 @@
   private Config _dynamicConfig;
   private JobState _jobState;
 
+  // Preventing Helix calling cancel before taskAttempt is created
+  // Checking if taskAttempt is empty is not enough, since canceller runs in different thread as runner, the case to
+  // to avoided here is taskAttempt being created and start to run after cancel has been called.
 
 Review comment:
   nit: the case to be avoided?

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

autumnust commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#discussion_r391275587
 
 

 ##########
 File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/SingleTask.java
 ##########
 @@ -95,7 +108,16 @@ public void run()
         .createDefaultTopLevelBroker(jobConfig, GobblinScopeTypes.GLOBAL.defaultScopeInstance())) {
       SharedResourcesBroker<GobblinScopeTypes> jobBroker = getJobBroker(_jobState, globalBroker);
 
-      _taskAttempt = _taskAttemptBuilder.build(workUnits.iterator(), _jobId, _jobState, jobBroker);
+      // Secure atomicity of taskAttempt's execution.
+      // Signaling blocking threads if any whenever taskAttempt is nonNull.
+      _lock.lock();
+      try {
+        _taskAttempt = _taskAttemptBuilder.build(getWorkUnits().iterator(), _jobId, _jobState, jobBroker);
+        _taskAttemptBuilt.signal();
+      } finally {
+        _lock.unlock();
+      }
+
       _taskAttempt.runAndOptionallyCommitTaskAttempt(GobblinMultiTaskAttempt.CommitPolicy.IMMEDIATE);
 
 Review comment:
   I think it is fine, the way that cancel works is, it call `shutdownTasks()` which actually sets a flag for each task, indicating the task has been terminated externally. This signal will be checked in extractor as it pull data from upstream, whenever it is being detected, it accomplish the ongoing loop of pulling data.  So the `run()` method within `runAndOptionallyCommitTaskAttempt` should still return. 
   
   But I don't have test case to verify this though, I might be too aggressive for pruning critical section since it is not performance sensitive at all. Will put it back. 

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] ZihanLi58 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

ZihanLi58 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#discussion_r395222680
 
 

 ##########
 File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/SingleTask.java
 ##########
 @@ -93,8 +109,17 @@ public void run()
         .createDefaultTopLevelBroker(jobConfig, GobblinScopeTypes.GLOBAL.defaultScopeInstance())) {
       SharedResourcesBroker<GobblinScopeTypes> jobBroker = getJobBroker(_jobState, globalBroker);
 
-      _taskAttempt = _taskAttemptBuilder.build(workUnits.iterator(), _jobId, _jobState, jobBroker);
-      _taskAttempt.runAndOptionallyCommitTaskAttempt(GobblinMultiTaskAttempt.CommitPolicy.IMMEDIATE);
+      // Secure atomicity of taskAttempt's execution.
+      // Signaling blocking threads if any whenever taskAttempt is nonNull.
+      _taskAttempt = _taskAttemptBuilder.build(getWorkUnits().iterator(), _jobId, _jobState, jobBroker);
+
+      _lock.lock();
+      try {
+        _taskAttemptBuilt.signal();
+        _taskAttempt.runAndOptionallyCommitTaskAttempt(GobblinMultiTaskAttempt.CommitPolicy.IMMEDIATE);
 
 Review comment:
   I was mistakenly thinking runAndOptionallyCommitTaskAttempt will return immediately. Then how about move this method out and make the build of taskAttemp inside the lock block?

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] sv2000 commented on issue #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

sv2000 commented on issue #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#issuecomment-601279473
 
 
   @autumnust - On thinking more about the synchronization problem between GobblinMultiTaskAttempt#run() and cancel() methods, I think the following strategy might work. You can create a countdownLatch object that is shared between the run() and cancel() threads. When a cancel() is invoked, the countDownLatch is incremented. The cancel() may proceed to perform other actions but before it returns, it blocks until this latch is down to 0. The run() thread on the other hand, detects that the latch has been incremented, cancels the tasks submitted to the task executor, decrements the latch and returns. This should solve the synchronization issues between the threads without needing a lock/condition variable. 

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] ZihanLi58 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

ZihanLi58 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#discussion_r390667675
 
 

 ##########
 File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/SingleTask.java
 ##########
 @@ -160,16 +181,32 @@ protected JobState getJobState()
   }
 
   public void cancel() {
-    if (_taskAttempt != null) {
+    int retryCount = 0 ;
+    int maxRetry = this._dynamicConfig.hasPath(MAX_RETRY_WAITING_FOR_INIT_KEY)
+        ? this._dynamicConfig.getInt(MAX_RETRY_WAITING_FOR_INIT_KEY) : DEFAULT_MAX_RETRY_WAITING_FOR_INIT;
+
+    try {
+      _lock.lock();
       try {
+        while (_taskAttempt == null) {
+          // await return false if timeout on this around
+          if (!_taskAttemptBuilt.await(5, TimeUnit.SECONDS) && ++retryCount > maxRetry) {
+            throw new IllegalStateException("Failed to initialize taskAttempt object before cancel");
 
 Review comment:
   Will this fail the job? Or we just want to log the error and ignore.

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

sv2000 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#discussion_r404242206
 
 

 ##########
 File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/SingleTask.java
 ##########
 @@ -164,16 +192,31 @@ protected JobState getJobState()
   }
 
   public void cancel() {
-    if (_taskAttempt != null) {
+    int retryCount = 0 ;
+    int maxRetry = ConfigUtils.getInt(_dynamicConfig, MAX_RETRY_WAITING_FOR_INIT_KEY, DEFAULT_MAX_RETRY_WAITING_FOR_INIT);
+
+    try {
+      _lock.lock();
       try {
+        while (_taskAttempt == null) {
+          // await return false if timeout on this around
+          if (!_taskAttemptBuilt.await(5, TimeUnit.SECONDS) && ++retryCount > maxRetry) {
+            throw new IllegalStateException("Failed to initialize taskAttempt object before cancel");
+          }
+        }
+      } finally {
+        _lock.unlock();
+      }
+
+      if (_taskAttempt != null) {
         _logger.info("Task cancelled: Shutdown starting for tasks with jobId: {}", _jobId);
         _taskAttempt.shutdownTasks();
         _logger.info("Task cancelled: Shutdown complete for tasks with jobId: {}", _jobId);
-      } catch (InterruptedException e) {
-        throw new RuntimeException("Interrupted while shutting down task with jobId: " + _jobId, e);
+      } else {
+        throw new IllegalStateException("This should never happen: TaskAttempt not initialized while passing conditional barrier");
 
 Review comment:
   Drop "This should never happen" from the message.

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

autumnust commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#discussion_r391035848
 
 

 ##########
 File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/SingleTask.java
 ##########
 @@ -63,6 +69,12 @@
   private Config _dynamicConfig;
   private JobState _jobState;
 
+  // Preventing Helix calling cancel before taskAttempt is created
+  // Checking if taskAttempt is empty is not enough, since canceller runs in different thread as runner, the case to
+  // to avoided here is taskAttempt being created and start to run after cancel has been called.
 
 Review comment:
   Addressed. 

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] autumnust commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

autumnust commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#discussion_r391039245
 
 

 ##########
 File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/SingleTask.java
 ##########
 @@ -160,16 +181,32 @@ protected JobState getJobState()
   }
 
   public void cancel() {
-    if (_taskAttempt != null) {
+    int retryCount = 0 ;
+    int maxRetry = this._dynamicConfig.hasPath(MAX_RETRY_WAITING_FOR_INIT_KEY)
+        ? this._dynamicConfig.getInt(MAX_RETRY_WAITING_FOR_INIT_KEY) : DEFAULT_MAX_RETRY_WAITING_FOR_INIT;
+
+    try {
+      _lock.lock();
       try {
+        while (_taskAttempt == null) {
+          // await return false if timeout on this around
+          if (!_taskAttemptBuilt.await(5, TimeUnit.SECONDS) && ++retryCount > maxRetry) {
+            throw new IllegalStateException("Failed to initialize taskAttempt object before cancel");
 
 Review comment:
   It will fail the cancel method and propagate exception back to Helix. 
   
   The reason of doing so, is because it is potentially dangerous to proceed in this case where the cancel was issue but the `taskAttempt` has not been initialized. For example it could result in two task running in the same helix instance in our streaming-ingestion pipeline where both task not falling behind due to the lack of enough resources

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

sv2000 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#discussion_r404243485
 
 

 ##########
 File path: gobblin-cluster/src/test/java/org/apache/gobblin/cluster/ClusterIntegrationTest.java
 ##########
 @@ -21,7 +21,21 @@
 import java.io.IOException;
 import java.nio.file.Paths;
 import java.util.Map;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.FutureTask;
 
+import org.apache.gobblin.cluster.suite.IntegrationBasicSuite;
 
 Review comment:
   Check the import order. org.apache.gobblin should come after com.google.*

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] autumnust commented on issue #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

autumnust commented on issue #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#issuecomment-597356434
 
 
   @ZihanLi58  Please take a look whenever you have time, thanks ! 

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] sv2000 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

sv2000 commented on a change in pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#discussion_r394050301
 
 

 ##########
 File path: gobblin-cluster/src/main/java/org/apache/gobblin/cluster/SingleTask.java
 ##########
 @@ -158,16 +183,32 @@ protected JobState getJobState()
   }
 
   public void cancel() {
-    if (_taskAttempt != null) {
+    int retryCount = 0 ;
+    int maxRetry = this._dynamicConfig.hasPath(MAX_RETRY_WAITING_FOR_INIT_KEY)
 
 Review comment:
   int maxRetry = ConfigUtils.get(this.dynamicConfig, MAX_RETRY_WAITING_FOR_INIT_KEY, DEFAULT_ MAX_RETRY_WAITING_FOR_INIT_KEY);?

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] codecov-io edited a comment on issue #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task
URL: https://github.com/apache/incubator-gobblin/pull/2919#issuecomment-598028158
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=h1) Report
   > Merging [#2919](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/7a328f9a232a60973d27c50859e6b84e63df90f7&el=desc) will **increase** coverage by `41.35%`.
   > The diff coverage is `78.94%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=tree)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #2919       +/-   ##
   =============================================
   + Coverage      4.12%   45.48%   +41.35%     
   - Complexity      750     9110     +8360     
   =============================================
     Files          1936     1936               
     Lines         73046    73058       +12     
     Branches       8046     8050        +4     
   =============================================
   + Hits           3016    33229    +30213     
   + Misses        69710    36772    -32938     
   - Partials        320     3057     +2737     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...a/org/apache/gobblin/cluster/GobblinHelixTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkhlbGl4VGFzay5qYXZh) | `77.61% <ø> (+77.61%)` | `6.00 <0.00> (+6.00)` | |
   | [...in/java/org/apache/gobblin/cluster/SingleTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2luZ2xlVGFzay5qYXZh) | `84.84% <78.94%> (+84.84%)` | `13.00 <5.00> (+13.00)` | |
   | [...gobblin/service/monitoring/JobStatusRetriever.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9Kb2JTdGF0dXNSZXRyaWV2ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `2.00% <0.00%> (+2.00%)` | |
   | [...ata/management/copy/hive/HivePartitionFileSet.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvaGl2ZS9IaXZlUGFydGl0aW9uRmlsZVNldC5qYXZh) | `0.95% <0.00%> (+0.95%)` | `1.00% <0.00%> (+1.00%)` | |
   | [...ain/java/org/apache/gobblin/runtime/TaskState.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvVGFza1N0YXRlLmphdmE=) | `81.97% <0.00%> (+1.16%)` | `32.00% <0.00%> (ø%)` | |
   | [...pache/gobblin/runtime/GobblinMultiTaskAttempt.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvR29iYmxpbk11bHRpVGFza0F0dGVtcHQuamF2YQ==) | `57.32% <0.00%> (+1.25%)` | `28.00% <0.00%> (+2.00%)` | |
   | [...ava/org/apache/gobblin/runtime/MultiConverter.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvTXVsdGlDb252ZXJ0ZXIuamF2YQ==) | `83.60% <0.00%> (+1.63%)` | `9.00% <0.00%> (+1.00%)` | |
   | [...rg/apache/gobblin/runtime/FsDatasetStateStore.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvRnNEYXRhc2V0U3RhdGVTdG9yZS5qYXZh) | `73.80% <0.00%> (+1.78%)` | `35.00% <0.00%> (+1.00%)` | |
   | [.../java/org/apache/gobblin/runtime/TaskExecutor.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvVGFza0V4ZWN1dG9yLmphdmE=) | `45.35% <0.00%> (+2.73%)` | `9.00% <0.00%> (+1.00%)` | |
   | [...org/apache/gobblin/yarn/GobblinYarnTaskRunner.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vR29iYmxpbllhcm5UYXNrUnVubmVyLmphdmE=) | `2.98% <0.00%> (+2.98%)` | `1.00% <0.00%> (+1.00%)` | |
   | ... and [1112 more](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=footer). Last update [7a328f9...751e0a1](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

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


With regards,
Apache Git Services

[GitHub] [incubator-gobblin] codecov-commenter edited a comment on pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

codecov-commenter edited a comment on pull request #2919:
URL: https://github.com/apache/incubator-gobblin/pull/2919#issuecomment-637828289


   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=h1) Report
   > Merging [#2919](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/2747b8d31f022388fc6808d0fbea5177c343c644&el=desc) will **increase** coverage by `4.26%`.
   > The diff coverage is `77.77%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #2919      +/-   ##
   ============================================
   + Coverage     41.47%   45.73%   +4.26%     
   - Complexity     8466     9296     +830     
   ============================================
     Files          1951     1951              
     Lines         74241    74252      +11     
     Branches       8215     8218       +3     
   ============================================
   + Hits          30790    33960    +3170     
   + Misses        40533    37120    -3413     
   - Partials       2918     3172     +254     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...a/org/apache/gobblin/cluster/GobblinHelixTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkhlbGl4VGFzay5qYXZh) | `77.61% <ø> (+2.98%)` | `6.00 <0.00> (ø)` | |
   | [...in/java/org/apache/gobblin/cluster/SingleTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2luZ2xlVGFzay5qYXZh) | `82.35% <76.47%> (+10.42%)` | `12.00 <4.00> (+3.00)` | |
   | [.../java/org/apache/gobblin/cluster/SleepingTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2xlZXBpbmdUYXNrLmphdmE=) | `39.39% <100.00%> (ø)` | `3.00 <0.00> (ø)` | |
   | [.../modules/scheduler/GobblinServiceJobScheduler.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9zY2hlZHVsZXIvR29iYmxpblNlcnZpY2VKb2JTY2hlZHVsZXIuamF2YQ==) | `57.14% <0.00%> (-2.12%)` | `24.00% <0.00%> (-2.00%)` | |
   | [...e/gobblin/runtime/app/ServiceBasedAppLauncher.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvYXBwL1NlcnZpY2VCYXNlZEFwcExhdW5jaGVyLmphdmE=) | `47.57% <0.00%> (-1.95%)` | `12.00% <0.00%> (ø%)` | |
   | [...src/main/java/org/apache/gobblin/runtime/Task.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvVGFzay5qYXZh) | `67.26% <0.00%> (+0.22%)` | `84.00% <0.00%> (+1.00%)` | |
   | [.../org/apache/gobblin/runtime/SafeDatasetCommit.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvU2FmZURhdGFzZXRDb21taXQuamF2YQ==) | `47.31% <0.00%> (+0.48%)` | `29.00% <0.00%> (ø%)` | |
   | [...ache/gobblin/cluster/GobblinHelixMultiManager.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkhlbGl4TXVsdGlNYW5hZ2VyLmphdmE=) | `53.64% <0.00%> (+0.52%)` | `17.00% <0.00%> (+1.00%)` | |
   | [...main/java/org/apache/gobblin/util/ConfigUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvQ29uZmlnVXRpbHMuamF2YQ==) | `58.86% <0.00%> (+0.63%)` | `41.00% <0.00%> (+1.00%)` | |
   | [...rg/apache/gobblin/runtime/AbstractJobLauncher.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvQWJzdHJhY3RKb2JMYXVuY2hlci5qYXZh) | `60.36% <0.00%> (+0.77%)` | `37.00% <0.00%> (+2.00%)` | |
   | ... and [154 more](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=footer). Last update [2747b8d...6434220](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

[GitHub] [incubator-gobblin] asfgit closed pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

asfgit closed pull request #2919:
URL: https://github.com/apache/incubator-gobblin/pull/2919


   


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

[GitHub] [incubator-gobblin] codecov-commenter commented on pull request #2919: [GOBBLIN-1078] Coordination between task cancel and initialization in Helix Task

Posted by GitBox <gi...@apache.org>.

codecov-commenter commented on pull request #2919:
URL: https://github.com/apache/incubator-gobblin/pull/2919#issuecomment-637828289


   # [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=h1) Report
   > Merging [#2919](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/2747b8d31f022388fc6808d0fbea5177c343c644&el=desc) will **decrease** coverage by `32.18%`.
   > The diff coverage is `33.33%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/graphs/tree.svg?width=650&height=150&src=pr&token=4MgURJ0bGc)](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #2919       +/-   ##
   ============================================
   - Coverage     41.47%   9.28%   -32.19%     
   + Complexity     8466    1695     -6771     
   ============================================
     Files          1951    1951               
     Lines         74241   74252       +11     
     Branches       8215    8218        +3     
   ============================================
   - Hits          30790    6895    -23895     
   - Misses        40533   66683    +26150     
   + Partials       2918     674     -2244     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...a/org/apache/gobblin/cluster/GobblinHelixTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkhlbGl4VGFzay5qYXZh) | `73.13% <ø> (-1.50%)` | `5.00 <0.00> (-1.00)` | |
   | [.../java/org/apache/gobblin/cluster/SleepingTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2xlZXBpbmdUYXNrLmphdmE=) | `0.00% <0.00%> (-39.40%)` | `0.00 <0.00> (-3.00)` | |
   | [...in/java/org/apache/gobblin/cluster/SingleTask.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2luZ2xlVGFzay5qYXZh) | `64.70% <35.29%> (-7.23%)` | `8.00 <0.00> (-1.00)` | |
   | [...c/main/java/org/apache/gobblin/util/FileUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvRmlsZVV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-4.00%)` | |
   | [...n/java/org/apache/gobblin/fork/CopyableSchema.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2ZvcmsvQ29weWFibGVTY2hlbWEuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-2.00%)` | |
   | [...java/org/apache/gobblin/stream/ControlMessage.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc3RyZWFtL0NvbnRyb2xNZXNzYWdlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
   | [...va/org/apache/gobblin/dataset/DatasetResolver.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YXNldC9EYXRhc2V0UmVzb2x2ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
   | [...va/org/apache/gobblin/converter/EmptyIterable.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1jb3JlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NvbnZlcnRlci9FbXB0eUl0ZXJhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
   | [...org/apache/gobblin/ack/BasicAckableForTesting.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vYWNrL0Jhc2ljQWNrYWJsZUZvclRlc3RpbmcuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-3.00%)` | |
   | [.../org/apache/gobblin/yarn/HelixMessageSubTypes.java](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree#diff-Z29iYmxpbi15YXJuL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3lhcm4vSGVsaXhNZXNzYWdlU3ViVHlwZXMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | `0.00% <0.00%> (-1.00%)` | |
   | ... and [1140 more](https://codecov.io/gh/apache/incubator-gobblin/pull/2919/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=footer). Last update [2747b8d...6434220](https://codecov.io/gh/apache/incubator-gobblin/pull/2919?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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