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/12/16 23:15:06 UTC
[GitHub] [incubator-gobblin] jack-moseley opened a new pull request #3179: [GOBBLIN-1342] Add API to resume a flow
jack-moseley opened a new pull request #3179:
URL: https://github.com/apache/incubator-gobblin/pull/3179
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
- [x] 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-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-1342
### Description
- [x] Here are some details about my PR, including screenshots (if applicable):
This PR adds flow resume functionality, which means after a flow fails, resume can be called on it, and the flow will be reattempted from the point before it failed (i.e. not rerunning the successful jobs).
How it works:
- Whenever a flow fails, on cleanup, it is now added to a `failedDagStateStore`, similar to the normal dag state store. (It is also kept in an in-memory `failedDags` map to avoid querying the store for every resume request).
- User triggers a resume request by calling partial update on the FlowExecutions endpoint, updating the status from `FAILED` to `RUNNING`. This is a little confusing, but as per [this discussion](https://softwareengineering.stackexchange.com/questions/261552/which-http-verb-should-i-use-to-trigger-an-action-in-a-rest-web-service) it seems to be the most correct way to represent a triggered action like this in rest.
- The request is forwarded to the `DagManager` as a `ResumeFlowEvent` through the eventbus, similar to how killing a flow works.
- `DagManager` adds it to a `resumeQueue` to be picked up by a thread
- The thread calls `beginResumingDag` on the dag, which sets the status of both the flow and each FAILED/CANCELLED job to `PENDING_RESUME` (and also sends events so this will be updated in the status store). The dag is then added to another map `resumingDags`
- In `finishResumingDags`, the dags in `resumingDags` are checked by querying the status store. If the dag has reached the correct state, then it is passed back to `initialize` to be launched. This is separated from `beginResumingDag` because it takes some time for the status to be updated in the status store.
Other related changes:
- Fixed a bug where `addDag` sets all jobs to `PENDING`, even if the dag is already running. This would cause their initial jobs to get relaunched on service restart.
- Added a `flowStartTime` variable in `JobExecutionPlan` which is set on flow resume so that flow SLA will not be calculated based on the original flow.
- Added `PENDING_RESUME` to execution status enum, it is a special status required because resuming flows are allowed to move "backwards" in status from `FAILED` to `PENDING`.
### Tests
- [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
Add unit test
### Commits
- [x] 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
[GitHub] [incubator-gobblin] codecov-io edited a comment on pull request #3179: [GOBBLIN-1342] Add API to resume a flow
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #3179:
URL: https://github.com/apache/incubator-gobblin/pull/3179#issuecomment-757108686
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=h1) Report
> Merging [#3179](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=desc) (bcbdf77) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/92fe239355e5c0354a95c2a1ff93e7afc2f53cae?el=desc) (92fe239) will **decrease** coverage by `0.01%`.
> The diff coverage is `49.19%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3179 +/- ##
============================================
- Coverage 46.19% 46.17% -0.02%
- Complexity 9721 9773 +52
============================================
Files 2006 2018 +12
Lines 76856 77326 +470
Branches 8548 8586 +38
============================================
+ Hits 35500 35704 +204
- Misses 38060 38304 +244
- Partials 3296 3318 +22
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../org/apache/gobblin/metrics/event/TimingEvent.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1tZXRyaWNzLWxpYnMvZ29iYmxpbi1tZXRyaWNzLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0cmljcy9ldmVudC9UaW1pbmdFdmVudC5qYXZh) | `70.00% <ø> (ø)` | `15.00 <0.00> (ø)` | |
| [...rg/apache/gobblin/service/FlowExecutionClient.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93RXhlY3V0aW9uQ2xpZW50LmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [.../apache/gobblin/service/FlowExecutionResource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93RXhlY3V0aW9uUmVzb3VyY2UuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...lin/service/FlowExecutionResourceLocalHandler.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93RXhlY3V0aW9uUmVzb3VyY2VMb2NhbEhhbmRsZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...he/gobblin/service/monitoring/ResumeFlowEvent.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9SZXN1bWVGbG93RXZlbnQuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...in/service/modules/orchestration/Orchestrator.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL09yY2hlc3RyYXRvci5qYXZh) | `48.93% <0.00%> (ø)` | `14.00 <0.00> (ø)` | |
| [...li/GobblinServiceFlowExecutionResourceHandler.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9yZXN0bGkvR29iYmxpblNlcnZpY2VGbG93RXhlY3V0aW9uUmVzb3VyY2VIYW5kbGVyLmphdmE=) | `30.43% <0.00%> (-13.32%)` | `2.00 <0.00> (ø)` | |
| [...n/service/modules/spec/SerializationConstants.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9zcGVjL1NlcmlhbGl6YXRpb25Db25zdGFudHMuamF2YQ==) | `0.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [.../service/monitoring/KafkaAvroJobStatusMonitor.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9LYWZrYUF2cm9Kb2JTdGF0dXNNb25pdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...gobblin/service/modules/spec/JobExecutionPlan.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9zcGVjL0pvYkV4ZWN1dGlvblBsYW4uamF2YQ==) | `67.10% <50.00%> (+0.43%)` | `10.00 <2.00> (+1.00)` | |
| ... and [37 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?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/3179?src=pr&el=footer). Last update [92fe239...bcbdf77](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?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] codecov-io edited a comment on pull request #3179: [GOBBLIN-1342] Add API to resume a flow
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #3179:
URL: https://github.com/apache/incubator-gobblin/pull/3179#issuecomment-757108686
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=h1) Report
> Merging [#3179](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=desc) (bcbdf77) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/92fe239355e5c0354a95c2a1ff93e7afc2f53cae?el=desc) (92fe239) will **decrease** coverage by `0.01%`.
> The diff coverage is `49.19%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3179 +/- ##
============================================
- Coverage 46.19% 46.17% -0.02%
- Complexity 9721 9773 +52
============================================
Files 2006 2018 +12
Lines 76856 77326 +470
Branches 8548 8586 +38
============================================
+ Hits 35500 35704 +204
- Misses 38060 38304 +244
- Partials 3296 3318 +22
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../org/apache/gobblin/metrics/event/TimingEvent.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1tZXRyaWNzLWxpYnMvZ29iYmxpbi1tZXRyaWNzLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0cmljcy9ldmVudC9UaW1pbmdFdmVudC5qYXZh) | `70.00% <ø> (ø)` | `15.00 <0.00> (ø)` | |
| [...rg/apache/gobblin/service/FlowExecutionClient.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93RXhlY3V0aW9uQ2xpZW50LmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [.../apache/gobblin/service/FlowExecutionResource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93RXhlY3V0aW9uUmVzb3VyY2UuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...lin/service/FlowExecutionResourceLocalHandler.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93RXhlY3V0aW9uUmVzb3VyY2VMb2NhbEhhbmRsZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...he/gobblin/service/monitoring/ResumeFlowEvent.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9SZXN1bWVGbG93RXZlbnQuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...in/service/modules/orchestration/Orchestrator.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL09yY2hlc3RyYXRvci5qYXZh) | `48.93% <0.00%> (ø)` | `14.00 <0.00> (ø)` | |
| [...li/GobblinServiceFlowExecutionResourceHandler.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9yZXN0bGkvR29iYmxpblNlcnZpY2VGbG93RXhlY3V0aW9uUmVzb3VyY2VIYW5kbGVyLmphdmE=) | `30.43% <0.00%> (-13.32%)` | `2.00 <0.00> (ø)` | |
| [...n/service/modules/spec/SerializationConstants.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9zcGVjL1NlcmlhbGl6YXRpb25Db25zdGFudHMuamF2YQ==) | `0.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
| [.../service/monitoring/KafkaAvroJobStatusMonitor.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9LYWZrYUF2cm9Kb2JTdGF0dXNNb25pdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...gobblin/service/modules/spec/JobExecutionPlan.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9zcGVjL0pvYkV4ZWN1dGlvblBsYW4uamF2YQ==) | `67.10% <50.00%> (+0.43%)` | `10.00 <2.00> (+1.00)` | |
| ... and [37 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?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/3179?src=pr&el=footer). Last update [92fe239...bcbdf77](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?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] sv2000 commented on a change in pull request #3179: [GOBBLIN-1342] Add API to resume a flow
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3179:
URL: https://github.com/apache/incubator-gobblin/pull/3179#discussion_r554219344
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -462,6 +501,64 @@ public void run() {
}
}
+ /**
+ * Begin resuming a dag by setting the status of both the dag and the failed/cancelled dag nodes to {@link ExecutionStatus#PENDING_RESUME},
+ * and also sending events so that this status will be reflected in the job status state store.
+ */
+ private void beginResumingDag(String dagId) {
+ Dag<JobExecutionPlan> dag = this.failedDags.get(dagId);
+ if (dag == null) {
+ log.warn("No dag found with dagId " + dagId + ", so cannot resume flow");
+ return;
+ }
+
+ long flowResumeTime = System.currentTimeMillis();
+
+ // Set the flow and it's failed or cancelled nodes to PENDING_RESUME so that the flow will be resumed from the point before it failed
+ DagManagerUtils.emitFlowEvent(this.eventSubmitter, dag, TimingEvent.FlowTimings.FLOW_PENDING_RESUME);
+ for (DagNode<JobExecutionPlan> node : dag.getNodes()) {
+ ExecutionStatus executionStatus = node.getValue().getExecutionStatus();
+ if (executionStatus.equals(FAILED) || executionStatus.equals(CANCELLED)) {
+ node.getValue().setExecutionStatus(PENDING_RESUME);
+ }
+ Map<String, String> jobMetadata = TimingEventUtils.getJobMetadata(Maps.newHashMap(), node.getValue());
+ this.eventSubmitter.get().getTimingEvent(TimingEvent.LauncherTimings.JOB_PENDING_RESUME).stop(jobMetadata);
+
+ // Set flowStartTime so that flow SLA will be based on current time instead of original flow
+ node.getValue().setFlowStartTime(flowResumeTime);
+ }
+
+ this.resumingDags.put(dagId, dag);
+ }
+
+ /**
+ * Finish resuming dags by first verifying the status is correct (flow should be {@link ExecutionStatus#PENDING_RESUME}
+ * and jobs should not be {@link ExecutionStatus#FAILED} or {@link ExecutionStatus#CANCELLED}) and then calling
+ * {@link #initialize}. This is separated from {@link #beginResumingDag} because it could take some time for the
+ * job status state store to reflect the updated status.
+ */
+ private void finishResumingDags() throws IOException {
+ for (Map.Entry<String, Dag<JobExecutionPlan>> dag : this.resumingDags.entrySet()) {
+ JobStatus flowStatus = pollFlowStatus(dag.getValue());
+ if (!flowStatus.getEventName().equals(PENDING_RESUME.name())) {
+ return;
+ }
+
+ for (DagNode<JobExecutionPlan> node : dag.getValue().getNodes()) {
+ JobStatus jobStatus = pollJobStatus(node);
+ if (jobStatus == null || jobStatus.getEventName().equals(FAILED.name()) || jobStatus.getEventName().equals(CANCELLED.name())) {
+ return;
Review comment:
Same comment as earlier.
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -462,6 +501,64 @@ public void run() {
}
}
+ /**
+ * Begin resuming a dag by setting the status of both the dag and the failed/cancelled dag nodes to {@link ExecutionStatus#PENDING_RESUME},
+ * and also sending events so that this status will be reflected in the job status state store.
+ */
+ private void beginResumingDag(String dagId) {
+ Dag<JobExecutionPlan> dag = this.failedDags.get(dagId);
+ if (dag == null) {
+ log.warn("No dag found with dagId " + dagId + ", so cannot resume flow");
+ return;
+ }
+
+ long flowResumeTime = System.currentTimeMillis();
+
+ // Set the flow and it's failed or cancelled nodes to PENDING_RESUME so that the flow will be resumed from the point before it failed
+ DagManagerUtils.emitFlowEvent(this.eventSubmitter, dag, TimingEvent.FlowTimings.FLOW_PENDING_RESUME);
+ for (DagNode<JobExecutionPlan> node : dag.getNodes()) {
+ ExecutionStatus executionStatus = node.getValue().getExecutionStatus();
+ if (executionStatus.equals(FAILED) || executionStatus.equals(CANCELLED)) {
+ node.getValue().setExecutionStatus(PENDING_RESUME);
+ }
+ Map<String, String> jobMetadata = TimingEventUtils.getJobMetadata(Maps.newHashMap(), node.getValue());
+ this.eventSubmitter.get().getTimingEvent(TimingEvent.LauncherTimings.JOB_PENDING_RESUME).stop(jobMetadata);
+
+ // Set flowStartTime so that flow SLA will be based on current time instead of original flow
+ node.getValue().setFlowStartTime(flowResumeTime);
+ }
+
+ this.resumingDags.put(dagId, dag);
+ }
+
+ /**
+ * Finish resuming dags by first verifying the status is correct (flow should be {@link ExecutionStatus#PENDING_RESUME}
+ * and jobs should not be {@link ExecutionStatus#FAILED} or {@link ExecutionStatus#CANCELLED}) and then calling
+ * {@link #initialize}. This is separated from {@link #beginResumingDag} because it could take some time for the
+ * job status state store to reflect the updated status.
+ */
+ private void finishResumingDags() throws IOException {
+ for (Map.Entry<String, Dag<JobExecutionPlan>> dag : this.resumingDags.entrySet()) {
+ JobStatus flowStatus = pollFlowStatus(dag.getValue());
+ if (!flowStatus.getEventName().equals(PENDING_RESUME.name())) {
Review comment:
Possible NPE here. Is there a possibility that flowStatus is null?
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -462,6 +501,64 @@ public void run() {
}
}
+ /**
+ * Begin resuming a dag by setting the status of both the dag and the failed/cancelled dag nodes to {@link ExecutionStatus#PENDING_RESUME},
+ * and also sending events so that this status will be reflected in the job status state store.
+ */
+ private void beginResumingDag(String dagId) {
+ Dag<JobExecutionPlan> dag = this.failedDags.get(dagId);
+ if (dag == null) {
+ log.warn("No dag found with dagId " + dagId + ", so cannot resume flow");
+ return;
+ }
+
+ long flowResumeTime = System.currentTimeMillis();
+
+ // Set the flow and it's failed or cancelled nodes to PENDING_RESUME so that the flow will be resumed from the point before it failed
+ DagManagerUtils.emitFlowEvent(this.eventSubmitter, dag, TimingEvent.FlowTimings.FLOW_PENDING_RESUME);
+ for (DagNode<JobExecutionPlan> node : dag.getNodes()) {
+ ExecutionStatus executionStatus = node.getValue().getExecutionStatus();
+ if (executionStatus.equals(FAILED) || executionStatus.equals(CANCELLED)) {
+ node.getValue().setExecutionStatus(PENDING_RESUME);
+ }
+ Map<String, String> jobMetadata = TimingEventUtils.getJobMetadata(Maps.newHashMap(), node.getValue());
+ this.eventSubmitter.get().getTimingEvent(TimingEvent.LauncherTimings.JOB_PENDING_RESUME).stop(jobMetadata);
+
+ // Set flowStartTime so that flow SLA will be based on current time instead of original flow
+ node.getValue().setFlowStartTime(flowResumeTime);
+ }
+
+ this.resumingDags.put(dagId, dag);
+ }
+
+ /**
+ * Finish resuming dags by first verifying the status is correct (flow should be {@link ExecutionStatus#PENDING_RESUME}
+ * and jobs should not be {@link ExecutionStatus#FAILED} or {@link ExecutionStatus#CANCELLED}) and then calling
+ * {@link #initialize}. This is separated from {@link #beginResumingDag} because it could take some time for the
+ * job status state store to reflect the updated status.
+ */
+ private void finishResumingDags() throws IOException {
+ for (Map.Entry<String, Dag<JobExecutionPlan>> dag : this.resumingDags.entrySet()) {
+ JobStatus flowStatus = pollFlowStatus(dag.getValue());
+ if (!flowStatus.getEventName().equals(PENDING_RESUME.name())) {
+ return;
Review comment:
Should this be continue instead of return? Don't you want to process the remiaining dags?
----------------------------------------------------------------
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-io commented on pull request #3179: [GOBBLIN-1342] Add API to resume a flow
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #3179:
URL: https://github.com/apache/incubator-gobblin/pull/3179#issuecomment-757108686
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=h1) Report
> Merging [#3179](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=desc) (bcbdf77) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/92fe239355e5c0354a95c2a1ff93e7afc2f53cae?el=desc) (92fe239) will **decrease** coverage by `37.09%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3179 +/- ##
============================================
- Coverage 46.19% 9.09% -37.10%
+ Complexity 9721 1727 -7994
============================================
Files 2006 2018 +12
Lines 76856 77326 +470
Branches 8548 8586 +38
============================================
- Hits 35500 7035 -28465
- Misses 38060 69603 +31543
+ Partials 3296 688 -2608
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../org/apache/gobblin/metrics/event/TimingEvent.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1tZXRyaWNzLWxpYnMvZ29iYmxpbi1tZXRyaWNzLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0cmljcy9ldmVudC9UaW1pbmdFdmVudC5qYXZh) | `0.00% <ø> (-70.00%)` | `0.00 <0.00> (-15.00)` | |
| [...rg/apache/gobblin/service/FlowExecutionClient.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93RXhlY3V0aW9uQ2xpZW50LmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [.../apache/gobblin/service/FlowExecutionResource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93RXhlY3V0aW9uUmVzb3VyY2UuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...lin/service/FlowExecutionResourceLocalHandler.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93RXhlY3V0aW9uUmVzb3VyY2VMb2NhbEhhbmRsZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...he/gobblin/service/monitoring/ResumeFlowEvent.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9SZXN1bWVGbG93RXZlbnQuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...blin/service/modules/orchestration/DagManager.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-71.05%)` | `0.00 <0.00> (-13.00)` | |
| [...service/modules/orchestration/DagManagerUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXJVdGlscy5qYXZh) | `0.00% <0.00%> (-83.52%)` | `0.00 <0.00> (-40.00)` | |
| [...in/service/modules/orchestration/Orchestrator.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL09yY2hlc3RyYXRvci5qYXZh) | `0.00% <0.00%> (-48.94%)` | `0.00 <0.00> (-14.00)` | |
| [...li/GobblinServiceFlowExecutionResourceHandler.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9yZXN0bGkvR29iYmxpblNlcnZpY2VGbG93RXhlY3V0aW9uUmVzb3VyY2VIYW5kbGVyLmphdmE=) | `0.00% <0.00%> (-43.75%)` | `0.00 <0.00> (-2.00)` | |
| [...gobblin/service/modules/spec/JobExecutionPlan.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9zcGVjL0pvYkV4ZWN1dGlvblBsYW4uamF2YQ==) | `0.00% <0.00%> (-66.67%)` | `0.00 <0.00> (-9.00)` | |
| ... and [1084 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?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/3179?src=pr&el=footer). Last update [92fe239...bcbdf77](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?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] jack-moseley commented on a change in pull request #3179: [GOBBLIN-1342] Add API to resume a flow
Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #3179:
URL: https://github.com/apache/incubator-gobblin/pull/3179#discussion_r554251417
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -462,6 +501,64 @@ public void run() {
}
}
+ /**
+ * Begin resuming a dag by setting the status of both the dag and the failed/cancelled dag nodes to {@link ExecutionStatus#PENDING_RESUME},
+ * and also sending events so that this status will be reflected in the job status state store.
+ */
+ private void beginResumingDag(String dagId) {
+ Dag<JobExecutionPlan> dag = this.failedDags.get(dagId);
+ if (dag == null) {
+ log.warn("No dag found with dagId " + dagId + ", so cannot resume flow");
+ return;
+ }
+
+ long flowResumeTime = System.currentTimeMillis();
+
+ // Set the flow and it's failed or cancelled nodes to PENDING_RESUME so that the flow will be resumed from the point before it failed
+ DagManagerUtils.emitFlowEvent(this.eventSubmitter, dag, TimingEvent.FlowTimings.FLOW_PENDING_RESUME);
+ for (DagNode<JobExecutionPlan> node : dag.getNodes()) {
+ ExecutionStatus executionStatus = node.getValue().getExecutionStatus();
+ if (executionStatus.equals(FAILED) || executionStatus.equals(CANCELLED)) {
+ node.getValue().setExecutionStatus(PENDING_RESUME);
+ }
+ Map<String, String> jobMetadata = TimingEventUtils.getJobMetadata(Maps.newHashMap(), node.getValue());
+ this.eventSubmitter.get().getTimingEvent(TimingEvent.LauncherTimings.JOB_PENDING_RESUME).stop(jobMetadata);
+
+ // Set flowStartTime so that flow SLA will be based on current time instead of original flow
+ node.getValue().setFlowStartTime(flowResumeTime);
+ }
+
+ this.resumingDags.put(dagId, dag);
+ }
+
+ /**
+ * Finish resuming dags by first verifying the status is correct (flow should be {@link ExecutionStatus#PENDING_RESUME}
+ * and jobs should not be {@link ExecutionStatus#FAILED} or {@link ExecutionStatus#CANCELLED}) and then calling
+ * {@link #initialize}. This is separated from {@link #beginResumingDag} because it could take some time for the
+ * job status state store to reflect the updated status.
+ */
+ private void finishResumingDags() throws IOException {
+ for (Map.Entry<String, Dag<JobExecutionPlan>> dag : this.resumingDags.entrySet()) {
+ JobStatus flowStatus = pollFlowStatus(dag.getValue());
+ if (!flowStatus.getEventName().equals(PENDING_RESUME.name())) {
+ return;
+ }
+
+ for (DagNode<JobExecutionPlan> node : dag.getValue().getNodes()) {
+ JobStatus jobStatus = pollJobStatus(node);
+ if (jobStatus == null || jobStatus.getEventName().equals(FAILED.name()) || jobStatus.getEventName().equals(CANCELLED.name())) {
+ return;
Review comment:
For this one continue doesn't work, since it is inside a second for loop, so I just added a dagReady flag
----------------------------------------------------------------
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 #3179: [GOBBLIN-1342] Add API to resume a flow
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3179:
URL: https://github.com/apache/incubator-gobblin/pull/3179
----------------------------------------------------------------
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] jack-moseley commented on a change in pull request #3179: [GOBBLIN-1342] Add API to resume a flow
Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #3179:
URL: https://github.com/apache/incubator-gobblin/pull/3179#discussion_r554251198
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -462,6 +501,64 @@ public void run() {
}
}
+ /**
+ * Begin resuming a dag by setting the status of both the dag and the failed/cancelled dag nodes to {@link ExecutionStatus#PENDING_RESUME},
+ * and also sending events so that this status will be reflected in the job status state store.
+ */
+ private void beginResumingDag(String dagId) {
+ Dag<JobExecutionPlan> dag = this.failedDags.get(dagId);
+ if (dag == null) {
+ log.warn("No dag found with dagId " + dagId + ", so cannot resume flow");
+ return;
+ }
+
+ long flowResumeTime = System.currentTimeMillis();
+
+ // Set the flow and it's failed or cancelled nodes to PENDING_RESUME so that the flow will be resumed from the point before it failed
+ DagManagerUtils.emitFlowEvent(this.eventSubmitter, dag, TimingEvent.FlowTimings.FLOW_PENDING_RESUME);
+ for (DagNode<JobExecutionPlan> node : dag.getNodes()) {
+ ExecutionStatus executionStatus = node.getValue().getExecutionStatus();
+ if (executionStatus.equals(FAILED) || executionStatus.equals(CANCELLED)) {
+ node.getValue().setExecutionStatus(PENDING_RESUME);
+ }
+ Map<String, String> jobMetadata = TimingEventUtils.getJobMetadata(Maps.newHashMap(), node.getValue());
+ this.eventSubmitter.get().getTimingEvent(TimingEvent.LauncherTimings.JOB_PENDING_RESUME).stop(jobMetadata);
+
+ // Set flowStartTime so that flow SLA will be based on current time instead of original flow
+ node.getValue().setFlowStartTime(flowResumeTime);
+ }
+
+ this.resumingDags.put(dagId, dag);
+ }
+
+ /**
+ * Finish resuming dags by first verifying the status is correct (flow should be {@link ExecutionStatus#PENDING_RESUME}
+ * and jobs should not be {@link ExecutionStatus#FAILED} or {@link ExecutionStatus#CANCELLED}) and then calling
+ * {@link #initialize}. This is separated from {@link #beginResumingDag} because it could take some time for the
+ * job status state store to reflect the updated status.
+ */
+ private void finishResumingDags() throws IOException {
+ for (Map.Entry<String, Dag<JobExecutionPlan>> dag : this.resumingDags.entrySet()) {
+ JobStatus flowStatus = pollFlowStatus(dag.getValue());
+ if (!flowStatus.getEventName().equals(PENDING_RESUME.name())) {
+ return;
Review comment:
Good catch, yeah it should be continue.
----------------------------------------------------------------
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-io commented on pull request #3179: [GOBBLIN-1342] Add API to resume a flow
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #3179:
URL: https://github.com/apache/incubator-gobblin/pull/3179#issuecomment-757108686
# [Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=h1) Report
> Merging [#3179](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=desc) (bcbdf77) into [master](https://codecov.io/gh/apache/incubator-gobblin/commit/92fe239355e5c0354a95c2a1ff93e7afc2f53cae?el=desc) (92fe239) will **decrease** coverage by `37.09%`.
> The diff coverage is `0.00%`.
[](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #3179 +/- ##
============================================
- Coverage 46.19% 9.09% -37.10%
+ Complexity 9721 1727 -7994
============================================
Files 2006 2018 +12
Lines 76856 77326 +470
Branches 8548 8586 +38
============================================
- Hits 35500 7035 -28465
- Misses 38060 69603 +31543
+ Partials 3296 688 -2608
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [.../org/apache/gobblin/metrics/event/TimingEvent.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1tZXRyaWNzLWxpYnMvZ29iYmxpbi1tZXRyaWNzLWJhc2Uvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vbWV0cmljcy9ldmVudC9UaW1pbmdFdmVudC5qYXZh) | `0.00% <ø> (-70.00%)` | `0.00 <0.00> (-15.00)` | |
| [...rg/apache/gobblin/service/FlowExecutionClient.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93RXhlY3V0aW9uQ2xpZW50LmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [.../apache/gobblin/service/FlowExecutionResource.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93RXhlY3V0aW9uUmVzb3VyY2UuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...lin/service/FlowExecutionResourceLocalHandler.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1yZXN0bGkvZ29iYmxpbi1mbG93LWNvbmZpZy1zZXJ2aWNlL2dvYmJsaW4tZmxvdy1jb25maWctc2VydmljZS1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2dvYmJsaW4vc2VydmljZS9GbG93RXhlY3V0aW9uUmVzb3VyY2VMb2NhbEhhbmRsZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...he/gobblin/service/monitoring/ResumeFlowEvent.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9uaXRvcmluZy9SZXN1bWVGbG93RXZlbnQuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...blin/service/modules/orchestration/DagManager.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-71.05%)` | `0.00 <0.00> (-13.00)` | |
| [...service/modules/orchestration/DagManagerUtils.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL0RhZ01hbmFnZXJVdGlscy5qYXZh) | `0.00% <0.00%> (-83.52%)` | `0.00 <0.00> (-40.00)` | |
| [...in/service/modules/orchestration/Orchestrator.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9vcmNoZXN0cmF0aW9uL09yY2hlc3RyYXRvci5qYXZh) | `0.00% <0.00%> (-48.94%)` | `0.00 <0.00> (-14.00)` | |
| [...li/GobblinServiceFlowExecutionResourceHandler.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9yZXN0bGkvR29iYmxpblNlcnZpY2VGbG93RXhlY3V0aW9uUmVzb3VyY2VIYW5kbGVyLmphdmE=) | `0.00% <0.00%> (-43.75%)` | `0.00 <0.00> (-2.00)` | |
| [...gobblin/service/modules/spec/JobExecutionPlan.java](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree#diff-Z29iYmxpbi1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NlcnZpY2UvbW9kdWxlcy9zcGVjL0pvYkV4ZWN1dGlvblBsYW4uamF2YQ==) | `0.00% <0.00%> (-66.67%)` | `0.00 <0.00> (-9.00)` | |
| ... and [1084 more](https://codecov.io/gh/apache/incubator-gobblin/pull/3179/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?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/3179?src=pr&el=footer). Last update [92fe239...bcbdf77](https://codecov.io/gh/apache/incubator-gobblin/pull/3179?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] jack-moseley commented on a change in pull request #3179: [GOBBLIN-1342] Add API to resume a flow
Posted by GitBox <gi...@apache.org>.
jack-moseley commented on a change in pull request #3179:
URL: https://github.com/apache/incubator-gobblin/pull/3179#discussion_r554251198
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -462,6 +501,64 @@ public void run() {
}
}
+ /**
+ * Begin resuming a dag by setting the status of both the dag and the failed/cancelled dag nodes to {@link ExecutionStatus#PENDING_RESUME},
+ * and also sending events so that this status will be reflected in the job status state store.
+ */
+ private void beginResumingDag(String dagId) {
+ Dag<JobExecutionPlan> dag = this.failedDags.get(dagId);
+ if (dag == null) {
+ log.warn("No dag found with dagId " + dagId + ", so cannot resume flow");
+ return;
+ }
+
+ long flowResumeTime = System.currentTimeMillis();
+
+ // Set the flow and it's failed or cancelled nodes to PENDING_RESUME so that the flow will be resumed from the point before it failed
+ DagManagerUtils.emitFlowEvent(this.eventSubmitter, dag, TimingEvent.FlowTimings.FLOW_PENDING_RESUME);
+ for (DagNode<JobExecutionPlan> node : dag.getNodes()) {
+ ExecutionStatus executionStatus = node.getValue().getExecutionStatus();
+ if (executionStatus.equals(FAILED) || executionStatus.equals(CANCELLED)) {
+ node.getValue().setExecutionStatus(PENDING_RESUME);
+ }
+ Map<String, String> jobMetadata = TimingEventUtils.getJobMetadata(Maps.newHashMap(), node.getValue());
+ this.eventSubmitter.get().getTimingEvent(TimingEvent.LauncherTimings.JOB_PENDING_RESUME).stop(jobMetadata);
+
+ // Set flowStartTime so that flow SLA will be based on current time instead of original flow
+ node.getValue().setFlowStartTime(flowResumeTime);
+ }
+
+ this.resumingDags.put(dagId, dag);
+ }
+
+ /**
+ * Finish resuming dags by first verifying the status is correct (flow should be {@link ExecutionStatus#PENDING_RESUME}
+ * and jobs should not be {@link ExecutionStatus#FAILED} or {@link ExecutionStatus#CANCELLED}) and then calling
+ * {@link #initialize}. This is separated from {@link #beginResumingDag} because it could take some time for the
+ * job status state store to reflect the updated status.
+ */
+ private void finishResumingDags() throws IOException {
+ for (Map.Entry<String, Dag<JobExecutionPlan>> dag : this.resumingDags.entrySet()) {
+ JobStatus flowStatus = pollFlowStatus(dag.getValue());
+ if (!flowStatus.getEventName().equals(PENDING_RESUME.name())) {
+ return;
Review comment:
Good catch, yeah it should be continue.
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -462,6 +501,64 @@ public void run() {
}
}
+ /**
+ * Begin resuming a dag by setting the status of both the dag and the failed/cancelled dag nodes to {@link ExecutionStatus#PENDING_RESUME},
+ * and also sending events so that this status will be reflected in the job status state store.
+ */
+ private void beginResumingDag(String dagId) {
+ Dag<JobExecutionPlan> dag = this.failedDags.get(dagId);
+ if (dag == null) {
+ log.warn("No dag found with dagId " + dagId + ", so cannot resume flow");
+ return;
+ }
+
+ long flowResumeTime = System.currentTimeMillis();
+
+ // Set the flow and it's failed or cancelled nodes to PENDING_RESUME so that the flow will be resumed from the point before it failed
+ DagManagerUtils.emitFlowEvent(this.eventSubmitter, dag, TimingEvent.FlowTimings.FLOW_PENDING_RESUME);
+ for (DagNode<JobExecutionPlan> node : dag.getNodes()) {
+ ExecutionStatus executionStatus = node.getValue().getExecutionStatus();
+ if (executionStatus.equals(FAILED) || executionStatus.equals(CANCELLED)) {
+ node.getValue().setExecutionStatus(PENDING_RESUME);
+ }
+ Map<String, String> jobMetadata = TimingEventUtils.getJobMetadata(Maps.newHashMap(), node.getValue());
+ this.eventSubmitter.get().getTimingEvent(TimingEvent.LauncherTimings.JOB_PENDING_RESUME).stop(jobMetadata);
+
+ // Set flowStartTime so that flow SLA will be based on current time instead of original flow
+ node.getValue().setFlowStartTime(flowResumeTime);
+ }
+
+ this.resumingDags.put(dagId, dag);
+ }
+
+ /**
+ * Finish resuming dags by first verifying the status is correct (flow should be {@link ExecutionStatus#PENDING_RESUME}
+ * and jobs should not be {@link ExecutionStatus#FAILED} or {@link ExecutionStatus#CANCELLED}) and then calling
+ * {@link #initialize}. This is separated from {@link #beginResumingDag} because it could take some time for the
+ * job status state store to reflect the updated status.
+ */
+ private void finishResumingDags() throws IOException {
+ for (Map.Entry<String, Dag<JobExecutionPlan>> dag : this.resumingDags.entrySet()) {
+ JobStatus flowStatus = pollFlowStatus(dag.getValue());
+ if (!flowStatus.getEventName().equals(PENDING_RESUME.name())) {
+ return;
+ }
+
+ for (DagNode<JobExecutionPlan> node : dag.getValue().getNodes()) {
+ JobStatus jobStatus = pollJobStatus(node);
+ if (jobStatus == null || jobStatus.getEventName().equals(FAILED.name()) || jobStatus.getEventName().equals(CANCELLED.name())) {
+ return;
Review comment:
For this one continue doesn't work, since it is inside a second for loop, so I just added a dagReady flag
----------------------------------------------------------------
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] sv2000 commented on a change in pull request #3179: [GOBBLIN-1342] Add API to resume a flow
Posted by GitBox <gi...@apache.org>.
sv2000 commented on a change in pull request #3179:
URL: https://github.com/apache/incubator-gobblin/pull/3179#discussion_r554219344
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -462,6 +501,64 @@ public void run() {
}
}
+ /**
+ * Begin resuming a dag by setting the status of both the dag and the failed/cancelled dag nodes to {@link ExecutionStatus#PENDING_RESUME},
+ * and also sending events so that this status will be reflected in the job status state store.
+ */
+ private void beginResumingDag(String dagId) {
+ Dag<JobExecutionPlan> dag = this.failedDags.get(dagId);
+ if (dag == null) {
+ log.warn("No dag found with dagId " + dagId + ", so cannot resume flow");
+ return;
+ }
+
+ long flowResumeTime = System.currentTimeMillis();
+
+ // Set the flow and it's failed or cancelled nodes to PENDING_RESUME so that the flow will be resumed from the point before it failed
+ DagManagerUtils.emitFlowEvent(this.eventSubmitter, dag, TimingEvent.FlowTimings.FLOW_PENDING_RESUME);
+ for (DagNode<JobExecutionPlan> node : dag.getNodes()) {
+ ExecutionStatus executionStatus = node.getValue().getExecutionStatus();
+ if (executionStatus.equals(FAILED) || executionStatus.equals(CANCELLED)) {
+ node.getValue().setExecutionStatus(PENDING_RESUME);
+ }
+ Map<String, String> jobMetadata = TimingEventUtils.getJobMetadata(Maps.newHashMap(), node.getValue());
+ this.eventSubmitter.get().getTimingEvent(TimingEvent.LauncherTimings.JOB_PENDING_RESUME).stop(jobMetadata);
+
+ // Set flowStartTime so that flow SLA will be based on current time instead of original flow
+ node.getValue().setFlowStartTime(flowResumeTime);
+ }
+
+ this.resumingDags.put(dagId, dag);
+ }
+
+ /**
+ * Finish resuming dags by first verifying the status is correct (flow should be {@link ExecutionStatus#PENDING_RESUME}
+ * and jobs should not be {@link ExecutionStatus#FAILED} or {@link ExecutionStatus#CANCELLED}) and then calling
+ * {@link #initialize}. This is separated from {@link #beginResumingDag} because it could take some time for the
+ * job status state store to reflect the updated status.
+ */
+ private void finishResumingDags() throws IOException {
+ for (Map.Entry<String, Dag<JobExecutionPlan>> dag : this.resumingDags.entrySet()) {
+ JobStatus flowStatus = pollFlowStatus(dag.getValue());
+ if (!flowStatus.getEventName().equals(PENDING_RESUME.name())) {
+ return;
+ }
+
+ for (DagNode<JobExecutionPlan> node : dag.getValue().getNodes()) {
+ JobStatus jobStatus = pollJobStatus(node);
+ if (jobStatus == null || jobStatus.getEventName().equals(FAILED.name()) || jobStatus.getEventName().equals(CANCELLED.name())) {
+ return;
Review comment:
Same comment as earlier.
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -462,6 +501,64 @@ public void run() {
}
}
+ /**
+ * Begin resuming a dag by setting the status of both the dag and the failed/cancelled dag nodes to {@link ExecutionStatus#PENDING_RESUME},
+ * and also sending events so that this status will be reflected in the job status state store.
+ */
+ private void beginResumingDag(String dagId) {
+ Dag<JobExecutionPlan> dag = this.failedDags.get(dagId);
+ if (dag == null) {
+ log.warn("No dag found with dagId " + dagId + ", so cannot resume flow");
+ return;
+ }
+
+ long flowResumeTime = System.currentTimeMillis();
+
+ // Set the flow and it's failed or cancelled nodes to PENDING_RESUME so that the flow will be resumed from the point before it failed
+ DagManagerUtils.emitFlowEvent(this.eventSubmitter, dag, TimingEvent.FlowTimings.FLOW_PENDING_RESUME);
+ for (DagNode<JobExecutionPlan> node : dag.getNodes()) {
+ ExecutionStatus executionStatus = node.getValue().getExecutionStatus();
+ if (executionStatus.equals(FAILED) || executionStatus.equals(CANCELLED)) {
+ node.getValue().setExecutionStatus(PENDING_RESUME);
+ }
+ Map<String, String> jobMetadata = TimingEventUtils.getJobMetadata(Maps.newHashMap(), node.getValue());
+ this.eventSubmitter.get().getTimingEvent(TimingEvent.LauncherTimings.JOB_PENDING_RESUME).stop(jobMetadata);
+
+ // Set flowStartTime so that flow SLA will be based on current time instead of original flow
+ node.getValue().setFlowStartTime(flowResumeTime);
+ }
+
+ this.resumingDags.put(dagId, dag);
+ }
+
+ /**
+ * Finish resuming dags by first verifying the status is correct (flow should be {@link ExecutionStatus#PENDING_RESUME}
+ * and jobs should not be {@link ExecutionStatus#FAILED} or {@link ExecutionStatus#CANCELLED}) and then calling
+ * {@link #initialize}. This is separated from {@link #beginResumingDag} because it could take some time for the
+ * job status state store to reflect the updated status.
+ */
+ private void finishResumingDags() throws IOException {
+ for (Map.Entry<String, Dag<JobExecutionPlan>> dag : this.resumingDags.entrySet()) {
+ JobStatus flowStatus = pollFlowStatus(dag.getValue());
+ if (!flowStatus.getEventName().equals(PENDING_RESUME.name())) {
Review comment:
Possible NPE here. Is there a possibility that flowStatus is null?
##########
File path: gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/DagManager.java
##########
@@ -462,6 +501,64 @@ public void run() {
}
}
+ /**
+ * Begin resuming a dag by setting the status of both the dag and the failed/cancelled dag nodes to {@link ExecutionStatus#PENDING_RESUME},
+ * and also sending events so that this status will be reflected in the job status state store.
+ */
+ private void beginResumingDag(String dagId) {
+ Dag<JobExecutionPlan> dag = this.failedDags.get(dagId);
+ if (dag == null) {
+ log.warn("No dag found with dagId " + dagId + ", so cannot resume flow");
+ return;
+ }
+
+ long flowResumeTime = System.currentTimeMillis();
+
+ // Set the flow and it's failed or cancelled nodes to PENDING_RESUME so that the flow will be resumed from the point before it failed
+ DagManagerUtils.emitFlowEvent(this.eventSubmitter, dag, TimingEvent.FlowTimings.FLOW_PENDING_RESUME);
+ for (DagNode<JobExecutionPlan> node : dag.getNodes()) {
+ ExecutionStatus executionStatus = node.getValue().getExecutionStatus();
+ if (executionStatus.equals(FAILED) || executionStatus.equals(CANCELLED)) {
+ node.getValue().setExecutionStatus(PENDING_RESUME);
+ }
+ Map<String, String> jobMetadata = TimingEventUtils.getJobMetadata(Maps.newHashMap(), node.getValue());
+ this.eventSubmitter.get().getTimingEvent(TimingEvent.LauncherTimings.JOB_PENDING_RESUME).stop(jobMetadata);
+
+ // Set flowStartTime so that flow SLA will be based on current time instead of original flow
+ node.getValue().setFlowStartTime(flowResumeTime);
+ }
+
+ this.resumingDags.put(dagId, dag);
+ }
+
+ /**
+ * Finish resuming dags by first verifying the status is correct (flow should be {@link ExecutionStatus#PENDING_RESUME}
+ * and jobs should not be {@link ExecutionStatus#FAILED} or {@link ExecutionStatus#CANCELLED}) and then calling
+ * {@link #initialize}. This is separated from {@link #beginResumingDag} because it could take some time for the
+ * job status state store to reflect the updated status.
+ */
+ private void finishResumingDags() throws IOException {
+ for (Map.Entry<String, Dag<JobExecutionPlan>> dag : this.resumingDags.entrySet()) {
+ JobStatus flowStatus = pollFlowStatus(dag.getValue());
+ if (!flowStatus.getEventName().equals(PENDING_RESUME.name())) {
+ return;
Review comment:
Should this be continue instead of return? Don't you want to process the remiaining dags?
----------------------------------------------------------------
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