-
Notifications
You must be signed in to change notification settings - Fork 320
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
Runless events - consume dataset event #2641
Conversation
✅ Deploy Preview for peppy-sprite-186812 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov Report
@@ Coverage Diff @@
## main #2641 +/- ##
============================================
+ Coverage 83.35% 83.60% +0.25%
- Complexity 1295 1334 +39
============================================
Files 244 245 +1
Lines 5948 6076 +128
Branches 279 280 +1
============================================
+ Hits 4958 5080 +122
Misses 844 844
- Partials 146 152 +6
📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
0f8c78c
to
2dfc2c0
Compare
2dfc2c0
to
2cb3e37
Compare
bf33c31
to
2213b35
Compare
Signed-off-by: Pawel Leszczynski <[email protected]>
Signed-off-by: Pawel Leszczynski <[email protected]>
Signed-off-by: Pawel Leszczynski <[email protected]>
Signed-off-by: Pawel Leszczynski <[email protected]>
2213b35
to
40bfe6b
Compare
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.
I left some comments/questions but otherwise, this looks good.
I like the multiple commits. That made it so much easier to review.
null, | ||
facets)); | ||
|
||
// OutputFacets ... |
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.
Do we not have InputFacets as well in this case?
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.
Neither inputFacet
nor outputFacets
should be allowed for DatasetEvent
.
We should have 3 methods: insertDatasetFacets
, insertInputDatasetFacets
and insertOutputDatasetFacets
.
I fixed this in the 5th commit.
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.
Ahh yes thanks for clarifying; was thinking about just this during my review 👀
Signed-off-by: Pawel Leszczynski <[email protected]>
Thanks for the update! |
ColumnLineageDao columnLineageDao = createColumnLineageDao(); | ||
RunFacetsDao runFacetsDao = createRunFacetsDao(); | ||
default UpdateLineageRow updateMarquezModel(DatasetEvent event, ObjectMapper mapper) { | ||
daos.initBaseDao(this); |
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.
This is pretty cool! I agree a wrapper class like ModelDaos
helps with readability and minimizes DAO
boilerplate initialization code. We can probably take the refactoring further an ask ourselves if we really need all of this logic in the DAO layer? Anyways, great to see cleanup efforts in this part of the codebase! 💯
api/src/main/resources/marquez/db/migration/V65__dataset_facets_lineage_event_type_nullable.sql
Show resolved
Hide resolved
Signed-off-by: Pawel Leszczynski <[email protected]>
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.
💯 💯 💯
Problem
Part of the #2624 to support
DatasetEvent
.Closes: #2624
Solution
Please describe your change as it relates to the problem, or bug fix, as well as any dependencies. If your change requires a database schema migration, please describe the schema modification(s) and whether it's a backwards-incompatible or backwards-compatible change.
One-line summary:
Checklist
CHANGELOG.md
(Depending on the change, this may not be necessary)..sql
database schema migration according to Flyway's naming convention (if relevant)