[GOBBLIN-1453] Improve error reporting on failed flow compilations and fix bugs wher… by Will-Lo · Pull Request #3291 · apache/gobblin

Will-Lo · 2021-05-27T20:08:38Z

…e failed flow compilations can be saved

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 issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-XXX

Description

Here are some details about my PR, including screenshots (if applicable):
Flowconfigs that fail to compile and return 400 errors should report why
Fix bug where templates that fail to compile would reject flow configs (even if they are not relevant)

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

Will-Lo · 2021-05-27T20:09:35Z

@jack-moseley @arjun4084346 @aplex @umustafi Please take a look

codecov-commenter · 2021-05-27T20:34:40Z

Codecov Report

Merging #3291 (6c8b559) into master (ea522eb) will decrease coverage by 37.54%.
The diff coverage is 0.00%.

❗ Current head 6c8b559 differs from pull request most recent head 85baab3. Consider uploading reports for the commit 85baab3 to get more accurate results

@@             Coverage Diff              @@
##             master   #3291       +/-   ##
============================================
- Coverage     46.53%   8.99%   -37.55%     
+ Complexity    10031    1742     -8289     
============================================
  Files          2037    2037               
  Lines         79278   79295       +17     
  Branches       8841    8840        -1     
============================================
- Hits          36894    7134    -29760     
- Misses        38959   71458    +32499     
+ Partials       3425     703     -2722

Impacted Files	Coverage Δ
...org/apache/gobblin/metrics/ServiceMetricNames.java	`0.00% <ø> (ø)`
...ache/gobblin/runtime/spec_catalog/FlowCatalog.java	`0.00% <0.00%> (-50.35%)`	⬇️
...lin/service/modules/flow/MultiHopFlowCompiler.java	`0.00% <0.00%> (-70.93%)`	⬇️
...dules/flowgraph/pathfinder/AbstractPathFinder.java	`0.00% <0.00%> (-87.07%)`	⬇️
...blin/service/modules/orchestration/DagManager.java	`0.00% <0.00%> (-72.04%)`	⬇️
.../modules/scheduler/GobblinServiceJobScheduler.java	`0.00% <ø> (-57.87%)`	⬇️
...c/main/java/org/apache/gobblin/util/FileUtils.java	`0.00% <0.00%> (-100.00%)`	⬇️
...n/java/org/apache/gobblin/fork/CopyableSchema.java	`0.00% <0.00%> (-100.00%)`	⬇️
...java/org/apache/gobblin/stream/ControlMessage.java	`0.00% <0.00%> (-100.00%)`	⬇️
...va/org/apache/gobblin/dataset/DatasetResolver.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 1084 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea522eb...85baab3. Read the comment docs.

gobblin-service/src/main/java/org/apache/gobblin/service/modules/flow/MultiHopFlowCompiler.java

...e/src/main/java/org/apache/gobblin/service/modules/scheduler/GobblinServiceJobScheduler.java

gobblin-runtime/src/main/java/org/apache/gobblin/runtime/spec_catalog/FlowCatalog.java

arjun4084346 · 2021-05-28T19:47:39Z

gobblin-runtime/src/main/java/org/apache/gobblin/runtime/spec_catalog/FlowCatalog.java

-        }
+      // If flow fails compilation, the result will have a non-empty string with the error
+      for (Map.Entry<SpecCatalogListener, CallbackResult<AddSpecResponse>> entry : response.getValue().getSuccesses().entrySet()) {
+        responseMap.put(entry.getKey().getName(), entry.getValue().getResult());


Please help me understand how the listeners work here. When a listener decides that compilation passes, it sends a Dag, it compilation fails, it returns null. In both the cases will the response be a success? What I mean is do we need to look at response.getValue().getFailures() in any case?

We do not need to look at response.getValue().getFailures() as long as no uncaught exceptions occur. It might be better if we set the default response from the listener to be a failure, so I'll go ahead and make those changes. So in the following scenarios:

If flow compilation from the listener is successful, return a dag in the AddSpecResponse

If flow compilation from the listener is unsuccessful, return null.

If flow compilation from the listener cannot be found, response should be null.

I think (3) is what caused an error with saving uncompilable flow specs before.

Or we throw an exception on every compilation error that occurs, that can work too. Except that the template resolution exceptions for flow edges can be unrelated to flow so those shouldn't be thrown.

…e failed flow compilations can be saved

umustafi

LGTM except for the other tests that are failing. They look like they mostly have to do with missing configs in the spec store (perhaps because of the change in behavior for bad specs)?

arjun4084346 · 2021-06-03T16:54:43Z

+1 LGTM

Will-Lo · 2021-06-03T16:57:19Z

@sv2000 Could you review/merge?

…d fix bugs wher… Closes apache#3291 from Will-Lo/modify-flow-compilation- error-reporting

jack-moseley reviewed May 28, 2021

View reviewed changes

gobblin-service/src/main/java/org/apache/gobblin/service/modules/flow/MultiHopFlowCompiler.java Show resolved Hide resolved

arjun4084346 reviewed May 28, 2021

View reviewed changes

...e/src/main/java/org/apache/gobblin/service/modules/scheduler/GobblinServiceJobScheduler.java Show resolved Hide resolved

arjun4084346 reviewed May 28, 2021

View reviewed changes

gobblin-runtime/src/main/java/org/apache/gobblin/runtime/spec_catalog/FlowCatalog.java Show resolved Hide resolved

arjun4084346 reviewed May 28, 2021

View reviewed changes

Will-Lo added 3 commits June 1, 2021 14:55

Improve error reporting on failed flow compilations and fix bugs wher…

fd92a14

…e failed flow compilations can be saved

Fix tests

1a7c66d

Address review

809f775

Will-Lo force-pushed the modify-flow-compilation-error-reporting branch from a6d7b97 to 809f775 Compare June 1, 2021 21:56

umustafi reviewed Jun 2, 2021

View reviewed changes

Will-Lo force-pushed the modify-flow-compilation-error-reporting branch from a656297 to 85baab3 Compare June 2, 2021 19:08

fix tests

3cba393

Will-Lo force-pushed the modify-flow-compilation-error-reporting branch from 85baab3 to 3cba393 Compare June 2, 2021 21:31

jack-moseley approved these changes Jun 2, 2021

View reviewed changes

arjun4084346 approved these changes Jun 3, 2021

View reviewed changes

asfgit closed this in 381b659 Jun 4, 2021

jhsenjaliya pushed a commit to jhsenjaliya/incubator-gobblin that referenced this pull request Jun 7, 2021

[GOBBLIN-1453] Improve error reporting on failed flow compilations an…

ca2b104

…d fix bugs wher… Closes apache#3291 from Will-Lo/modify-flow-compilation- error-reporting

Will-Lo deleted the modify-flow-compilation-error-reporting branch June 7, 2021 23:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GOBBLIN-1453] Improve error reporting on failed flow compilations and fix bugs wher…#3291

[GOBBLIN-1453] Improve error reporting on failed flow compilations and fix bugs wher…#3291
Will-Lo wants to merge 4 commits intoapache:masterfrom
Will-Lo:modify-flow-compilation-error-reporting

Will-Lo commented May 27, 2021 •

edited

Loading

Uh oh!

Will-Lo commented May 27, 2021 •

edited

Loading

Uh oh!

codecov-commenter commented May 27, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arjun4084346 May 28, 2021

Uh oh!

Will-Lo May 29, 2021

Uh oh!

umustafi left a comment

Uh oh!

arjun4084346 commented Jun 3, 2021

Uh oh!

Will-Lo commented Jun 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

Will-Lo commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA

Description

Tests

Commits

Uh oh!

Will-Lo commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arjun4084346 May 28, 2021

Choose a reason for hiding this comment

Uh oh!

Will-Lo May 29, 2021

Choose a reason for hiding this comment

Uh oh!

umustafi left a comment

Choose a reason for hiding this comment

Uh oh!

arjun4084346 commented Jun 3, 2021

Uh oh!

Will-Lo commented Jun 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Will-Lo commented May 27, 2021 •

edited

Loading

Will-Lo commented May 27, 2021 •

edited

Loading

codecov-commenter commented May 27, 2021 •

edited

Loading