Remove job context. by JDarDagran · Pull Request #2373 · MarquezProject/marquez

JDarDagran · 2023-01-20T10:43:08Z

Signed-off-by: Jakub Dardzinski kuba0221@gmail.com

Add call to API in frontend.
Add tests.

Signed-off-by: Jakub Dardzinski kuba0221@gmail.com

Problem

Closes: #2230

Solution

This PR removes use of job context. It also adds 2 endpoints for job/run facets per run. Those are called from web components to replace job context with SQLJobFacet.

There is also a lot of code removal. It appears that plenty of code was marked as uncovered due to @NonNull annotation and caused issues to achieve code coverage goal. According to lombok docs I've added lombok.nonNull.exceptionType=JDK to skip coverage checks on those annotations.

Checklist

You've signed-off your work
Your changes are accompanied by tests (if relevant)
Your change contains a small diff and is self-contained
You've updated any relevant documentation (if relevant)
You've updated the CHANGELOG.md with details about your change under the "Unreleased" section (if relevant, depending on the change, this may not be necessary)
You've versioned your .sql database schema migration according to Flyway's naming convention (if relevant)
You've included a header in any source code files (if relevant)

codecov · 2023-01-20T11:12:35Z

Codecov Report

Merging #2373 (d1af84c) into main (dbdbcc3) will increase coverage by 6.49%.
The diff coverage is 90.47%.

@@             Coverage Diff              @@
##               main    #2373      +/-   ##
============================================
+ Coverage     77.11%   83.61%   +6.49%     
+ Complexity     1234     1214      -20     
============================================
  Files           228      231       +3     
  Lines          5572     5522      -50     
  Branches        447      266     -181     
============================================
+ Hits           4297     4617     +320     
+ Misses          775      762      -13     
+ Partials        500      143     -357

Impacted Files	Coverage Δ
...i/src/main/java/marquez/api/models/JobVersion.java	`100.00% <ø> (+29.41%)`	⬆️
api/src/main/java/marquez/common/Utils.java	`93.91% <ø> (+5.25%)`	⬆️
...src/main/java/marquez/common/models/DatasetId.java	`100.00% <ø> (+18.18%)`	⬆️
api/src/main/java/marquez/db/Columns.java	`79.74% <ø> (ø)`
api/src/main/java/marquez/db/JobDao.java	`100.00% <ø> (ø)`
api/src/main/java/marquez/db/JobFacetsDao.java	`100.00% <ø> (+35.71%)`	⬆️
api/src/main/java/marquez/db/JobVersionDao.java	`98.38% <ø> (+7.34%)`	⬆️
api/src/main/java/marquez/db/RunFacetsDao.java	`100.00% <ø> (+19.04%)`	⬆️
...arquez/db/mappers/ExtendedJobVersionRowMapper.java	`80.00% <ø> (+7.27%)`	⬆️
...rc/main/java/marquez/db/mappers/JobDataMapper.java	`100.00% <ø> (+12.50%)`	⬆️
... and 142 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pawel-big-lebowski · 2023-01-20T13:44:34Z

+      """
+    SELECT
+      run_uuid,
+      JSON_AGG(event -> 'run' -> 'facets' ORDER BY event_time) AS facets


if we were able to merge #2355 before, it would be great to make use of run_facets_view.

@JDarDagran we were able to ;)

pawel-big-lebowski · 2023-01-20T13:47:55Z

+
+@EqualsAndHashCode
+@ToString
+public final class Facets {


If it is run related, then consider calling it RunFacets.

mobuchowski · 2023-01-23T10:17:22Z

@@ -0,0 +1,3 @@
+/* SPDX-License-Identifier: Apache-2.0 */
+
+ALTER TABLE job_versions DROP COLUMN job_context_uuid;


Let's drop column in the next version - this would prevent rollback if the feature would be buggy.

wslulciuc

@JDarDagran I think adding an endpoint for obtaining facets for a given run ID is a great addition to the Marquez API, but can we introduce the endpoint in a separate PR (to limit scope)? I've also provided some feedback on the runs facet endpoint.

wslulciuc · 2023-01-31T08:56:19Z

+  @ExceptionMetered
+  @GET
+  @Produces(APPLICATION_JSON)
+  @Path("/run/{id}")


I would add the endpoint to obtain facets for a given run to RunResource. That is, to receive a run, we have the endpoint:

GET /api/v1/jobs/runs/{id}

Similarly, to obtain facets for a given run, I would follow the same pattern:

GET /api/v1/jobs/runs/{id}/facets

JDarDagran · 2023-01-31T11:02:50Z

@JDarDagran I think adding an endpoint for obtaining facets for a given run ID is a great addition to the Marquez API, but can we introduce the endpoint in a separate PR (to limit scope)?

@wslulciuc Normally I would agree. However, if we don't add job facet endpoint in this PR we have no viable source of data to replace job context with. The closest one would be /api/v1/events/lineage but:

it does not allow to filter per run id
already has performance issues. It would add more if we'd need to filter results from the endpoint to match given run id (as per 1.)
For that reason I think this PR would less readable + less performant when introducing the endpoints in separate PR.

I do agree with your feedback on run facets endpoint, will apply to it.

tito12 · 2023-02-01T20:12:13Z

Also I am getting error on Graph view - "Something went wrong while fetching job facets". Maybe I don't have some data?

pawel-big-lebowski

I cannot speak for webbev part, but the backend part seems good to me.
I've included some minor comments.
I think Java client is missing Run/Job facets added in service package, however this can be added later on in separate PR as this one already got pretty big.

🚀 👍 💯

Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com> Add call to API in frontend. Add tests. Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com> Update javadocs. Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com> Apply spotless fixes. Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com> Remove FacetResource. Split react functions to each component. Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com> Add more tests for JobResource. Update OpenAPI spec. Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com> Add more tests. Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com> Add lombok option to skip coverage check on NonNull. Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com> Move find facets methods to corresponding daos. Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>

wslulciuc

Thanks for the clean up @JDarDagran! 🥇 💯 🚀

tito12

Looks nice :)

Add call to API in frontend. Add tests. Update javadocs. Apply spotless fixes. Remove FacetResource. Split react functions to each component. Add more tests for JobResource. Update OpenAPI spec. Add more tests. Add lombok option to skip coverage check on NonNull. Move find facets methods to corresponding daos. Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com> Co-authored-by: Willy Lulciuc <willy@datakin.com>

boring-cyborg Bot added the api API layer changes label Jan 20, 2023

JDarDagran force-pushed the replace-job-context branch 5 times, most recently from d9daa4e to dbbec0c Compare January 20, 2023 11:04

pawel-big-lebowski reviewed Jan 20, 2023

View reviewed changes

mobuchowski reviewed Jan 23, 2023

View reviewed changes

wslulciuc requested changes Jan 31, 2023

View reviewed changes

wslulciuc reviewed Jan 31, 2023

View reviewed changes

tito12 reviewed Feb 1, 2023

View reviewed changes

JDarDagran force-pushed the replace-job-context branch 5 times, most recently from 6e51401 to 2742344 Compare February 19, 2023 22:14

pawel-big-lebowski approved these changes Feb 20, 2023

View reviewed changes

Comment thread api/src/main/java/marquez/db/RunDao.java Outdated

Comment thread api/src/main/java/marquez/db/RunDao.java Outdated

JDarDagran force-pushed the replace-job-context branch 7 times, most recently from 83304d4 to 3f7ce31 Compare February 21, 2023 08:36

JDarDagran requested a review from wslulciuc February 21, 2023 08:50

JDarDagran force-pushed the replace-job-context branch from 3f7ce31 to 95c00c8 Compare February 21, 2023 09:54

JDarDagran requested review from tito12 and removed request for wslulciuc February 21, 2023 10:00

Merge branch 'main' into replace-job-context

d1af84c

wslulciuc approved these changes Feb 22, 2023

View reviewed changes

tito12 approved these changes Feb 22, 2023

View reviewed changes

pawel-big-lebowski merged commit 79a9c4f into MarquezProject:main Feb 22, 2023

JDarDagran mentioned this pull request Feb 22, 2023

Add facets endpoint to Marquez clients. #2431

Open

wslulciuc mentioned this pull request Sep 23, 2023

Drop table job_contexts and usage #2621

Merged

		@@ -0,0 +1,3 @@
		/* SPDX-License-Identifier: Apache-2.0 */

		ALTER TABLE job_versions DROP COLUMN job_context_uuid;

Conversation

JDarDagran commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Checklist

Uh oh!

codecov Bot commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pawel-big-lebowski Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wslulciuc Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

pawel-big-lebowski Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

mobuchowski Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

wslulciuc left a comment

Choose a reason for hiding this comment

Uh oh!

wslulciuc Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JDarDagran commented Jan 31, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tito12 commented Feb 1, 2023

Uh oh!

pawel-big-lebowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wslulciuc left a comment

Choose a reason for hiding this comment

Uh oh!

tito12 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JDarDagran commented Jan 20, 2023 •

edited

Loading

codecov Bot commented Jan 20, 2023 •

edited

Loading

pawel-big-lebowski Jan 20, 2023 •

edited

Loading

wslulciuc Jan 31, 2023 •

edited

Loading