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 2022/10/13 18:02:29 UTC

[GitHub] [gobblin] hanghangliu opened a new pull request, #3580: give option to cancel helix workflow through Delete API

hanghangliu opened a new pull request, #3580:
URL: https://github.com/apache/gobblin/pull/3580

   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
   - [ ] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-1721] My Gobblin PR"
       - https://issues.apache.org/jira/browse/GOBBLIN-1721
   
   
   ### Description
   - [ ] Here are some details about my PR, including screenshots (if applicable):
   Currently when we receive a job restart(handleUpdateJobConfigArrival), GobblinHelixJobLauncher will firstly call  helixTaskDriver.waitToStop to stop the workflow, then initiate the new one. We observe the behavior of Helix taking exceptionally long to stop the workflow, making the job state staying in STOPPING status. This will make waitToStop timeout and throw exception all the time, making the new flow never be able to launch.
   
   We can utilize Delete API in this case since our job is stateless for Helix, to avoid job hanging.
   
   ### Tests
   - [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
   Tested in the cluster.
   
   ### 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.

To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org

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

[GitHub] [gobblin] hanghangliu commented on a diff in pull request #3580: [GOBBLIN-1721] Give option to cancel helix workflow through Delete API

Posted by GitBox <gi...@apache.org>.
hanghangliu commented on code in PR #3580:
URL: https://github.com/apache/gobblin/pull/3580#discussion_r998518634


##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixDistributeJobExecutionLauncher.java:
##########
@@ -110,6 +110,8 @@ class GobblinHelixDistributeJobExecutionLauncher implements JobExecutionLauncher
   @Getter
   private DistributeJobMonitor jobMonitor;
 
+  private final Config configs;

Review Comment:
   Done



-- 
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: dev-unsubscribe@gobblin.apache.org

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

[GitHub] [gobblin] homatthew commented on a diff in pull request #3580: [GOBBLIN-1721] Give option to cancel helix workflow through Delete API

Posted by GitBox <gi...@apache.org>.
homatthew commented on code in PR #3580:
URL: https://github.com/apache/gobblin/pull/3580#discussion_r997652378


##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/HelixUtils.java:
##########
@@ -359,13 +369,16 @@ static void handleJobTimeout(String workFlowName, String jobName, HelixManager h
   private static void deleteStoppedHelixJob(HelixManager helixManager, String workFlowName, String jobName)
       throws InterruptedException {
     WorkflowContext workflowContext = TaskDriver.getWorkflowContext(helixManager, workFlowName);
-    while (workflowContext.getJobState(TaskUtil.getNamespacedJobName(workFlowName, jobName)) != STOPPED) {
+    while (workflowContext != null &&
+        workflowContext.getJobState(TaskUtil.getNamespacedJobName(workFlowName, jobName)) != STOPPED) {
       log.info("Waiting for job {} to stop...", jobName);
       workflowContext = TaskDriver.getWorkflowContext(helixManager, workFlowName);
       Thread.sleep(1000);
     }
-    // deleting the entire workflow, as one workflow contains only one job
-    new TaskDriver(helixManager).deleteAndWaitForCompletion(workFlowName, 10000L);
+    if (workflowContext != null) {

Review Comment:
   Maybe add a comment for future readers to understand the subtlety of these null checks



##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/HelixUtils.java:
##########
@@ -359,13 +369,16 @@ static void handleJobTimeout(String workFlowName, String jobName, HelixManager h
   private static void deleteStoppedHelixJob(HelixManager helixManager, String workFlowName, String jobName)
       throws InterruptedException {
     WorkflowContext workflowContext = TaskDriver.getWorkflowContext(helixManager, workFlowName);
-    while (workflowContext.getJobState(TaskUtil.getNamespacedJobName(workFlowName, jobName)) != STOPPED) {
+    while (workflowContext != null &&
+        workflowContext.getJobState(TaskUtil.getNamespacedJobName(workFlowName, jobName)) != STOPPED) {
       log.info("Waiting for job {} to stop...", jobName);
       workflowContext = TaskDriver.getWorkflowContext(helixManager, workFlowName);
       Thread.sleep(1000);
     }
-    // deleting the entire workflow, as one workflow contains only one job
-    new TaskDriver(helixManager).deleteAndWaitForCompletion(workFlowName, 10000L);
+    if (workflowContext != null) {
+      // deleting the entire workflow, as one workflow contains only one job
+      new TaskDriver(helixManager).deleteAndWaitForCompletion(workFlowName, 10000L);

Review Comment:
   I am okay with hard coding this timeout value to 10 seconds since the deletion has typically happened very fast. But can we have a separate variable and corresponding comment that explains that 10 seconds should be plenty of time and we should expect deletes to happen a few seconds?
   
   Just to avoid magic numbers and better readability for future readers



##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/HelixUtils.java:
##########
@@ -359,13 +369,16 @@ static void handleJobTimeout(String workFlowName, String jobName, HelixManager h
   private static void deleteStoppedHelixJob(HelixManager helixManager, String workFlowName, String jobName)
       throws InterruptedException {
     WorkflowContext workflowContext = TaskDriver.getWorkflowContext(helixManager, workFlowName);
-    while (workflowContext.getJobState(TaskUtil.getNamespacedJobName(workFlowName, jobName)) != STOPPED) {
+    while (workflowContext != null &&
+        workflowContext.getJobState(TaskUtil.getNamespacedJobName(workFlowName, jobName)) != STOPPED) {
       log.info("Waiting for job {} to stop...", jobName);
       workflowContext = TaskDriver.getWorkflowContext(helixManager, workFlowName);
       Thread.sleep(1000);
     }
-    // deleting the entire workflow, as one workflow contains only one job
-    new TaskDriver(helixManager).deleteAndWaitForCompletion(workFlowName, 10000L);
+    if (workflowContext != null) {

Review Comment:
   These null checks are to make sure the program continues this code doesn't fail in the delete scenario right?



##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/HelixUtils.java:
##########
@@ -323,6 +323,19 @@ static boolean isJobFinished(String workflowName, String jobName, HelixManager h
     }
   }
 
+  // Cancel the job by calling either Delete or Stop Helix API
+  public static void cancelWorkflow(String workflowName, HelixManager helixManager, long timeOut, boolean cancelByDelete)
+      throws InterruptedException {
+    TaskDriver taskDriver = new TaskDriver(helixManager);
+    if(cancelByDelete) {

Review Comment:
   nit: whitespace in between the if and `(`
   
   We probably should add a linter 😵‍💫 



-- 
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: dev-unsubscribe@gobblin.apache.org

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

[GitHub] [gobblin] codecov-commenter commented on pull request #3580: [GOBBLIN-1721] Give option to cancel helix workflow through Delete API

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3580:
URL: https://github.com/apache/gobblin/pull/3580#issuecomment-1278011638

   # [Codecov](https://codecov.io/gh/apache/gobblin/pull/3580?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#3580](https://codecov.io/gh/apache/gobblin/pull/3580?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d024c60) into [master](https://codecov.io/gh/apache/gobblin/commit/fafd40b25fdd3e8f8299f9e939794ad7a2492314?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fafd40b) will **increase** coverage by `1.78%`.
   > The diff coverage is `40.00%`.
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #3580      +/-   ##
   ============================================
   + Coverage     46.87%   48.66%   +1.78%     
   + Complexity    10640     7851    -2789     
   ============================================
     Files          2113     1467     -646     
     Lines         82798    57816   -24982     
     Branches       9220     6653    -2567     
   ============================================
   - Hits          38813    28134   -10679     
   + Misses        40425    27063   -13362     
   + Partials       3560     2619     -941     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3580?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...bblin/cluster/GobblinClusterConfigurationKeys.java](https://codecov.io/gh/apache/gobblin/pull/3580/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkNsdXN0ZXJDb25maWd1cmF0aW9uS2V5cy5qYXZh) | `0.00% <ø> (ø)` | |
   | [...ache/gobblin/cluster/GobblinHelixJobScheduler.java](https://codecov.io/gh/apache/gobblin/pull/3580/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkhlbGl4Sm9iU2NoZWR1bGVyLmphdmE=) | `53.67% <40.00%> (-0.93%)` | :arrow_down: |
   | [...a/org/apache/gobblin/util/limiter/NoopLimiter.java](https://codecov.io/gh/apache/gobblin/pull/3580/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvbGltaXRlci9Ob29wTGltaXRlci5qYXZh) | `40.00% <0.00%> (-20.00%)` | :arrow_down: |
   | [...lin/util/filesystem/FileSystemInstrumentation.java](https://codecov.io/gh/apache/gobblin/pull/3580/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvZmlsZXN5c3RlbS9GaWxlU3lzdGVtSW5zdHJ1bWVudGF0aW9uLmphdmE=) | `85.71% <0.00%> (-7.15%)` | :arrow_down: |
   | [.../java/org/apache/gobblin/cluster/SleepingTask.java](https://codecov.io/gh/apache/gobblin/pull/3580/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2xlZXBpbmdUYXNrLmphdmE=) | `39.39% <0.00%> (-6.07%)` | :arrow_down: |
   | [...a/management/copy/publisher/CopyDataPublisher.java](https://codecov.io/gh/apache/gobblin/pull/3580/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvcHVibGlzaGVyL0NvcHlEYXRhUHVibGlzaGVyLmphdmE=) | `74.17% <0.00%> (-1.33%)` | :arrow_down: |
   | [.../org/apache/gobblin/cluster/GobblinTaskRunner.java](https://codecov.io/gh/apache/gobblin/pull/3580/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpblRhc2tSdW5uZXIuamF2YQ==) | `63.63% <0.00%> (-0.32%)` | :arrow_down: |
   | [...anagement/copy/replication/ConfigBasedDataset.java](https://codecov.io/gh/apache/gobblin/pull/3580/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1kYXRhLW1hbmFnZW1lbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vZGF0YS9tYW5hZ2VtZW50L2NvcHkvcmVwbGljYXRpb24vQ29uZmlnQmFzZWREYXRhc2V0LmphdmE=) | `68.87% <0.00%> (ø)` | |
   | [...gobblin/service/monitoring/FsFlowGraphMonitor.java](https://codecov.io/gh/apache/gobblin/pull/3580/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9Gc0Zsb3dHcmFwaE1vbml0b3IuamF2YQ==) | | |
   | [.../apache/gobblin/http/ApacheHttpResponseStatus.java](https://codecov.io/gh/apache/gobblin/pull/3580/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4taHR0cC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9odHRwL0FwYWNoZUh0dHBSZXNwb25zZVN0YXR1cy5qYXZh) | | |
   | ... and [647 more](https://codecov.io/gh/apache/gobblin/pull/3580/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
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: dev-unsubscribe@gobblin.apache.org

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

[GitHub] [gobblin] homatthew commented on a diff in pull request #3580: [GOBBLIN-1721] Give option to cancel helix workflow through Delete API

Posted by GitBox <gi...@apache.org>.
homatthew commented on code in PR #3580:
URL: https://github.com/apache/gobblin/pull/3580#discussion_r997494999


##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixJobScheduler.java:
##########
@@ -431,8 +431,15 @@ private void cancelJobIfRequired(DeleteJobConfigArrivalEvent deleteJobArrival) t
       if (jobNameToWorkflowIdMap.containsKey(deleteJobArrival.getJobName())) {
         String workflowId = jobNameToWorkflowIdMap.get(deleteJobArrival.getJobName());
         TaskDriver taskDriver = new TaskDriver(this.jobHelixManager);
-        taskDriver.waitToStop(workflowId, this.helixJobStopTimeoutMillis);
-        LOGGER.info("Stopped workflow: {}", deleteJobArrival.getJobName());
+        // Cancel the job by calling either Delete or Stop Helix API
+        if (PropertiesUtils.getPropAsBoolean(jobConfig, GobblinClusterConfigurationKeys.CANCEL_HELIX_JOB_BY_DELETE,
+            GobblinClusterConfigurationKeys.DEFAULT_CANCEL_HELIX_JOB_BY_DELETE)) {
+          taskDriver.delete(workflowId);
+          LOGGER.info("Canceling Helix workflow: {} through delete API", deleteJobArrival.getJobName());
+        } else {

Review Comment:
   A few open questions for us to discuss:
   1. We use this waitToStop in other places. Should we consider using this delete api as a replacement for all calls? We use the taskrunner code basically everywhere and that is the rootcause for long stopping times.
   2. We should plan on cleaning up this config if we see no issues after further testing. Can we create a JIRA for this and add a comment for this.



-- 
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: dev-unsubscribe@gobblin.apache.org

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

[GitHub] [gobblin] ZihanLi58 merged pull request #3580: [GOBBLIN-1721] Give option to cancel helix workflow through Delete API

Posted by GitBox <gi...@apache.org>.
ZihanLi58 merged PR #3580:
URL: https://github.com/apache/gobblin/pull/3580


-- 
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: dev-unsubscribe@gobblin.apache.org

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

[GitHub] [gobblin] ZihanLi58 commented on a diff in pull request #3580: [GOBBLIN-1721] Give option to cancel helix workflow through Delete API

Posted by GitBox <gi...@apache.org>.
ZihanLi58 commented on code in PR #3580:
URL: https://github.com/apache/gobblin/pull/3580#discussion_r997607325


##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixDistributeJobExecutionLauncher.java:
##########
@@ -110,6 +110,8 @@ class GobblinHelixDistributeJobExecutionLauncher implements JobExecutionLauncher
   @Getter
   private DistributeJobMonitor jobMonitor;
 
+  private final Config configs;

Review Comment:
   Can we rename it to be combinedConfigs? 



-- 
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: dev-unsubscribe@gobblin.apache.org

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

[GitHub] [gobblin] hanghangliu commented on a diff in pull request #3580: [GOBBLIN-1721] Give option to cancel helix workflow through Delete API

Posted by GitBox <gi...@apache.org>.
hanghangliu commented on code in PR #3580:
URL: https://github.com/apache/gobblin/pull/3580#discussion_r998519055


##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinHelixJobScheduler.java:
##########
@@ -431,8 +431,15 @@ private void cancelJobIfRequired(DeleteJobConfigArrivalEvent deleteJobArrival) t
       if (jobNameToWorkflowIdMap.containsKey(deleteJobArrival.getJobName())) {
         String workflowId = jobNameToWorkflowIdMap.get(deleteJobArrival.getJobName());
         TaskDriver taskDriver = new TaskDriver(this.jobHelixManager);
-        taskDriver.waitToStop(workflowId, this.helixJobStopTimeoutMillis);
-        LOGGER.info("Stopped workflow: {}", deleteJobArrival.getJobName());
+        // Cancel the job by calling either Delete or Stop Helix API
+        if (PropertiesUtils.getPropAsBoolean(jobConfig, GobblinClusterConfigurationKeys.CANCEL_HELIX_JOB_BY_DELETE,
+            GobblinClusterConfigurationKeys.DEFAULT_CANCEL_HELIX_JOB_BY_DELETE)) {
+          taskDriver.delete(workflowId);
+          LOGGER.info("Canceling Helix workflow: {} through delete API", deleteJobArrival.getJobName());
+        } else {

Review Comment:
   Updated for all waitToStop use cases



-- 
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: dev-unsubscribe@gobblin.apache.org

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