optimizing current runs query for lieage api by prachim-collab · Pull Request #2211 · MarquezProject/marquez

prachim-collab · 2022-10-24T20:53:59Z

Problem

Introduce a simpler alternate getCurrentRuns query that gets only simple runs from DB without the additional data from tables such as run_args, job_context, facets, input/output versions etc which required the extra table joins in the old getCurrentRuns query. This new getCurrentRuns DAO is NOT being used in Marquez as of now.

Closes: #4425

Solution

The old getCurrentRuns DAO is renamed to getCurrentRunsWithFacets without any change to the sql query .
This new light weight query is call getCurrentRuns and is NOT called from /lineage api as of now so NO change is required to the /lineage api response spec.
A flag withRunFacets is also introduced as parameter to lineage api, which is always set to true to call getCurrentRunsWithFacets, so that /lineage api and hence all the tests still call the old DAO .

Note: All database schema changes require discussion. Please link the issue for context.

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)

boring-cyborg · 2022-10-24T20:54:02Z

Thanks for opening your first pull request in the Marquez project! Please check out our contributing guidelines (https://github.com/MarquezProject/marquez/blob/main/CONTRIBUTING.md).

codecov · 2022-10-24T20:59:46Z

Codecov Report

Merging #2211 (a330beb) into main (4b86615) will decrease coverage by 9.44%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #2211      +/-   ##
============================================
- Coverage     76.67%   67.23%   -9.45%     
+ Complexity     1144      227     -917     
============================================
  Files           219       40     -179     
  Lines          5312      940    -4372     
  Branches        421      102     -319     
============================================
- Hits           4073      632    -3441     
+ Misses          764      159     -605     
+ Partials        475      149     -326

Impacted Files	Coverage Δ
...src/main/java/marquez/api/OpenLineageResource.java
...pi/src/main/java/marquez/db/mappers/RunMapper.java
.../src/main/java/marquez/service/LineageService.java
...ain/java/marquez/service/ColumnLineageService.java
...i/src/main/java/marquez/service/SourceService.java
...c/main/java/marquez/api/ColumnLineageResource.java
...c/main/java/marquez/db/mappers/OwnerRowMapper.java
...i/src/main/java/marquez/graphql/mapper/RowMap.java
...pi/src/main/java/marquez/db/models/DatasetRow.java
...va/marquez/db/mappers/DatasetVersionRowMapper.java
... and 169 more

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

phixMe · 2022-10-24T21:52:56Z

    runAssert
        .extracting(Run::getOutputVersions, InstanceOfAssertFactories.list(DatasetVersionId.class))
-        .hasSize(1);
+        .hasSize(0);


I think this is a little misleading since we're not asking the database for this information anymore. Should we just remove this assertion or change the type definition?

I agree and had initially removed this completely but then added it back again, because getInputVersions also had an assertion. I guess i should remove both of them now.

fm100 · 2022-10-24T22:28:50Z

-    runAssert
-        .extracting(Run::getOutputVersions, InstanceOfAssertFactories.list(DatasetVersionId.class))
-        .hasSize(1);


We are asserting on the Lineage object returned from lineageService.lineage call, and it is the response payload of GET lineage API. Does this mean that API response is being changed?

Yeah, I don't like all the information being returned here, but breaking API compatibility is not good. If we want a lighter-weight version of the lineage API, I think it's better to either include an optional parameter to exclude the superfluous data or to create a new API and deprecate the old one

I have updated the PR and added a withRunFacets flag to get runs with all the superfluous data and this flag always true in Marquez apis, so no affect on the API compatibility.

collado-mike · 2022-11-02T18:23:30Z

  }

-  public Lineage lineage(NodeId nodeId, int depth) {
+  public Lineage lineage(NodeId nodeId, int depth, boolean withRunFacets) {


This is a good change, but I worry that we'll want to add more options to this method (e.g., include job facets? dataset facets? exclude runs altogether?). I don't think we should take this on now, but let's add a TODO to make the input parameters here more easily extendable so that we can add those other options later one.

Couldn't agree more. I had small aversion to adding a flag to make this work, but there was no other better option. I also thought in future if more changes like this come along that alter api significantly, we could add these as options to api query parameters, or create more broken apis to get specific data.

phixMe · 2022-11-02T18:28:50Z

+  @SqlQuery(
+      "SELECT DISTINCT on(r.job_name, r.namespace_name) r.*, jv.version as job_version\n"
+          + "    FROM runs_view r\n"
+          + "    INNER JOIN job_versions jv ON jv.uuid=r.job_version_uuid\n"
+          + "    INNER JOIN jobs_view j ON j.uuid=jv.job_uuid\n"
+          + "    WHERE j.uuid in (<jobUuid>) OR j.symlink_target_uuid IN (<jobUuid>)\n"
+          + "    ORDER BY r.job_name, r.namespace_name, created_at DESC\n")


I think we should use the more readable syntax variant as we update our queries.

""" SELECT DISTINCT on(r.job_name, r.namespace_name) r.*, jv.version as job_version FROM runs_view INNER JOIN job_versions jv ON jv.uuid=r.job_version_uuid INNER JOIN jobs_view j ON j.uuid=jv.job_uuid WHERE j.uuid in (<jobUuid>) OR j.symlink_target_uuid IN (<jobUuid>) ORDER BY r.job_name, r.namespace_name, created_at DESC """

updated the syntax as you asked

phixMe

Thanks so much!

Signed-off-by: Prachi Mishra <prachi.mishra@astronomer.io>

boring-cyborg · 2022-11-02T23:54:24Z

Great job! Congrats on your first merged pull request in the Marquez project!

* optimizing current runs query for lieage api Signed-off-by: Prachi Mishra <prachi.mishra@astronomer.io> * removing assert Signed-off-by: Prachi Mishra <prachi.mishra@astronomer.io> * add new lightweight current runs query Signed-off-by: Prachi Mishra <prachi.mishra@astronomer.io> * addressing review comments Signed-off-by: Prachi Mishra <prachi.mishra@astronomer.io> Signed-off-by: Prachi Mishra <prachi.mishra@astronomer.io>

boring-cyborg Bot added the api API layer changes label Oct 24, 2022

prachim-collab force-pushed the prachim/perf/4425_optimize_get_current_run_query branch from 6ca59e1 to bc2349d Compare October 24, 2022 21:23

prachim-collab marked this pull request as ready for review October 24, 2022 21:25

prachim-collab force-pushed the prachim/perf/4425_optimize_get_current_run_query branch 2 times, most recently from fea1168 to b39fcc5 Compare October 24, 2022 21:31

phixMe reviewed Oct 24, 2022

View reviewed changes

fm100 reviewed Oct 24, 2022

View reviewed changes

prachim-collab marked this pull request as draft October 26, 2022 20:51

prachim-collab force-pushed the prachim/perf/4425_optimize_get_current_run_query branch 2 times, most recently from 601278b to 52ebaa3 Compare October 31, 2022 22:12

prachim-collab marked this pull request as ready for review October 31, 2022 22:49

prachim-collab requested a review from phixMe November 1, 2022 16:51

harels requested a review from collado-mike November 1, 2022 18:31

collado-mike approved these changes Nov 2, 2022

View reviewed changes

phixMe reviewed Nov 2, 2022

View reviewed changes

phixMe approved these changes Nov 2, 2022

View reviewed changes

prachim-collab added 4 commits November 2, 2022 14:43

optimizing current runs query for lieage api

d62c272

Signed-off-by: Prachi Mishra <prachi.mishra@astronomer.io>

removing assert

08aeb43

Signed-off-by: Prachi Mishra <prachi.mishra@astronomer.io>

add new lightweight current runs query

8c1e395

Signed-off-by: Prachi Mishra <prachi.mishra@astronomer.io>

addressing review comments

a330beb

Signed-off-by: Prachi Mishra <prachi.mishra@astronomer.io>

prachim-collab force-pushed the prachim/perf/4425_optimize_get_current_run_query branch from 4662e51 to a330beb Compare November 2, 2022 21:43

collado-mike merged commit 6dad6aa into MarquezProject:main Nov 2, 2022

Conversation

prachim-collab commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Checklist

Uh oh!

boring-cyborg Bot commented Oct 24, 2022

Uh oh!

codecov Bot commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phixMe left a comment

Choose a reason for hiding this comment

Uh oh!

boring-cyborg Bot commented Nov 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

prachim-collab commented Oct 24, 2022 •

edited

Loading

codecov Bot commented Oct 24, 2022 •

edited

Loading