-
Notifications
You must be signed in to change notification settings - Fork 744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GOBBLIN-2137]merged dagNodeStateStore and failedDagNodeStateStore tables #4032
base: master
Are you sure you want to change the base?
[GOBBLIN-2137]merged dagNodeStateStore and failedDagNodeStateStore tables #4032
Conversation
e08ac21
to
719052f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4032 +/- ##
============================================
- Coverage 45.86% 38.80% -7.06%
+ Complexity 3257 1599 -1658
============================================
Files 707 388 -319
Lines 27865 15995 -11870
Branches 2796 1585 -1211
============================================
- Hits 12779 6207 -6572
+ Misses 14008 9290 -4718
+ Partials 1078 498 -580 ☔ View full report in Codecov by Sentry. |
…nto failed_dag_store_table_merger # Conflicts: # gobblin-service/src/main/java/org/apache/gobblin/service/modules/flowgraph/Dag.java # gobblin-service/src/main/java/org/apache/gobblin/service/modules/orchestration/MysqlDagStateStoreWithDagNodes.java # gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/MysqlDagStateStoreWithDagNodesTest.java
@@ -48,6 +48,8 @@ public class Dag<T> { | |||
// Map to maintain parent to children mapping. | |||
private Map<DagNode, List<DagNode<T>>> parentChildMap; | |||
private List<DagNode<T>> nodes; | |||
@Setter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we do not persist dag level field in mysql, adding fields to Dag will not be much useful and may lead to bugs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to have this field here as we didn't want to add additional parameters in all the methods to pass on is_failed value.
...rc/main/java/org/apache/gobblin/service/modules/orchestration/DagStateStoreWithDagNodes.java
Show resolved
Hide resolved
...in/java/org/apache/gobblin/service/modules/orchestration/MysqlDagStateStoreWithDagNodes.java
Outdated
Show resolved
Hide resolved
...in/java/org/apache/gobblin/service/modules/orchestration/MysqlDagStateStoreWithDagNodes.java
Outdated
Show resolved
Hide resolved
Should before completing the review, i would like to understand your thoughts on this. |
c13315d
to
7e3e556
Compare
If we don't keep it in mysql, we may lose it in case of restarts/deployments, so we will have to store in mysql |
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
Description
Right now we are maintaining two tables to maintain DagState and Failed Dag State, In this PR ,we have tried to merge FailedDagState tables into DagState by adding a column is_failed_dag in DagState
Tests
Commits