Add job tagging to API #2774

davidsharp7 · 2024-03-17T09:34:35Z

Problem

Currently we have no tag support for jobs.

Closes: #2756

Solution

Adding new table jobs_tag_mapping to store the relationship between jobs and tags.
Update jobs meta to return the tags as a list.
Add POST/DELETE end points to add and delete tags.

One-line summary:

Add job tagging to API

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 included a one-line summary of your change for the CHANGELOG.md (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)

Signed-off-by: sharpd <[email protected]>

netlify · 2024-03-17T09:34:50Z

✅ Deploy Preview for peppy-sprite-186812 canceled.

Name	Link
🔨 Latest commit	`bf6621e`
🔍 Latest deploy log	https://app.netlify.com/sites/peppy-sprite-186812/deploys/663a4ce80f1807000836ffb4

Signed-off-by: sharpd <[email protected]>

codecov · 2024-03-19T21:13:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.57%. Comparing base (ae794a9) to head (10703c1).

❗ Current head 10703c1 differs from pull request most recent head bf6621e. Consider uploading reports for the commit bf6621e to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2774      +/-   ##
============================================
+ Coverage     84.47%   84.57%   +0.09%     
- Complexity     1430     1441      +11     
============================================
  Files           251      251              
  Lines          6462     6503      +41     
  Branches        299      302       +3     
============================================
+ Hits           5459     5500      +41     
  Misses          850      850              
  Partials        153      153

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: sharpd <[email protected]>

phixMe · 2024-04-23T16:44:43Z

api/src/main/java/marquez/db/JobDao.java

+      NOW(),
+      NOW(),
+      :tagName,
+      'No Description'


Is there a reason we would want to hardcode this in the SQL, instead of in our application code to be a bit more flexible?

description is a nullable column from what I can see, I'm not sure we need to default it at all?

Yeah I can leave as null - was just being explicit more than anything but it is inconsistent with dataset/dataset field tagging so makes sense to set to null.

davidjgoss · 2024-04-23T16:42:37Z

api/src/main/java/marquez/api/JobResource.java

+    Job job =
+        jobService
+            .findJobByName(namespaceName.getValue(), jobName.getValue())
+            .orElseThrow(() -> new JobNotFoundException(jobName));


I guess this should be quite unlikely given updateJobTags would presumably throw if the job didn't exist? A case of, we get an optional back so we should do something beyond an unqualified get() on it?

Yeah understand what you mean -it will fall over before it gets to this point so why bother? Mainly as that seems to be the de-facto pattern for a lot of the code i.e

execute something -> retrieve object (job, dataset etc) else throw an error.

Thanks for following our code style!

api/src/main/java/marquez/db/JobDao.java

api/src/test/java/marquez/api/BaseResourceIntegrationTest.java

davidjgoss · 2024-04-23T16:55:16Z

clients/java/src/main/java/marquez/client/models/JobMeta.java

@@ -22,6 +22,7 @@ public class JobMeta {
  @Getter private final JobType type;
  @Getter private final Set<DatasetId> inputs;
  @Getter private final Set<DatasetId> outputs;
+  @Getter @NonNull private final Set<String> tags;


Disparity between @NonNull here and @Nullable in the constructor. The builder also doesn't specify - I think nullable makes sense though? For backwards-compatibility, especially.

davidjgoss · 2024-04-23T16:57:00Z

api/src/main/java/marquez/service/models/Job.java

@@ -43,6 +45,7 @@ public final class Job {
  @Getter private final ImmutableMap<String, Object> facets;
  @Nullable private UUID currentVersion;
  @Getter @Nullable private ImmutableList<String> labels;
+  @Getter private final ImmutableSet<TagName> tags;


Should we mark this @Nullable here? Would be consistent with other fields.

davidjgoss · 2024-04-23T16:59:27Z

clients/java/src/main/java/marquez/client/models/Job.java

@@ -33,6 +34,7 @@ public final class Job extends JobMeta {
  @Nullable private final Run latestRun;
  @Getter private final Map<String, Object> facets;
  @Nullable private final UUID currentVersion;
+  @Getter @NonNull private final Set<String> tags;


This class extends JobMeta which already defines tags as a field - I don't think we need to define it again here, but rather pass it through into the super ctor?

api/src/test/java/marquez/api/TagResourceIntegrationTest.java

Signed-off-by: sharpd <[email protected]>

…quez into api/add_job_tags

Signed-off-by: sharpd <[email protected]>

davidjgoss

LGTM. Not sure why webpack build is failing, as it runs okay for me locally. Could be it's maxing out resources?

davidsharp7 · 2024-04-25T17:45:56Z

It's a 137 exit code which is OOM so yes you out of resource. Did this a couple of weeks ago and then came right.

Signed-off-by: sharpd <[email protected]>

wslulciuc · 2024-05-07T02:22:10Z

api/src/main/java/marquez/api/JobResource.java

+    Job job =
+        jobService
+            .findJobByName(namespaceName.getValue(), jobName.getValue())
+            .orElseThrow(() -> new JobNotFoundException(jobName));


Thanks for following our code style!

api/src/main/java/marquez/db/JobDao.java

api/src/main/resources/marquez/db/migration/V68__add_jobs_tag_mapping.sql

Signed-off-by: sharpd <[email protected]>

wslulciuc

Excited to see tagging support for jobs! (And soon in the Web UI 🚀 )

add job tagging to API

0d44966

Signed-off-by: sharpd <[email protected]>

boring-cyborg bot added api API layer changes client/java labels Mar 17, 2024

fix various tests that were failing

d114c3f

Signed-off-by: sharpd <[email protected]>

Merge branch 'MarquezProject:main' into api/add_job_tags

fd76893

davidsharp7 marked this pull request as ready for review March 19, 2024 22:51

davidsharp7 and others added 7 commits March 21, 2024 18:32

Merge branch 'MarquezProject:main' into api/add_job_tags

655bf3a

Merge branch 'MarquezProject:main' into api/add_job_tags

02e7484

Merge branch 'main' into api/add_job_tags

58f40cf

fix merge commit

1d3f819

Signed-off-by: sharpd <[email protected]>

fix liniting

057bf14

Signed-off-by: sharpd <[email protected]>

Merge branch 'MarquezProject:main' into api/add_job_tags

4d1550a

Merge branch 'MarquezProject:main' into api/add_job_tags

da04b41

phixMe reviewed Apr 23, 2024

View reviewed changes

davidjgoss reviewed Apr 23, 2024

View reviewed changes

phixMe reviewed Apr 23, 2024

View reviewed changes

api/src/test/java/marquez/api/TagResourceIntegrationTest.java Outdated Show resolved Hide resolved

davidsharp7 and others added 6 commits April 24, 2024 06:25

Merge branch 'main' into api/add_job_tags

3f429f9

update based on pr feedback

911549b

Signed-off-by: sharpd <[email protected]>

Merge branch 'api/add_job_tags' of https://github.com/davidsharp7/mar…

2750d66

…quez into api/add_job_tags

lint tag test code

2b8ae9a

Signed-off-by: sharpd <[email protected]>

lint tag code

b73c6b3

Signed-off-by: sharpd <[email protected]>

fix log typo

ca332d6

Signed-off-by: sharpd <[email protected]>

davidjgoss reviewed Apr 25, 2024

View reviewed changes

davidsharp7 and others added 4 commits April 26, 2024 16:58

fix logging

322ed34

Signed-off-by: sharpd <[email protected]>

Merge branch 'MarquezProject:main' into api/add_job_tags

d127354

Merge branch 'MarquezProject:main' into api/add_job_tags

a64c2f6

Merge branch 'MarquezProject:main' into api/add_job_tags

4517062

Merge branch 'MarquezProject:main' into api/add_job_tags

f726170

davidsharp7 self-assigned this May 2, 2024

MarquezProject locked and limited conversation to collaborators May 3, 2024

MarquezProject unlocked this conversation May 3, 2024

davidsharp7 requested a review from wslulciuc May 5, 2024 05:32

wslulciuc reviewed May 7, 2024

View reviewed changes

davidsharp7 and others added 3 commits May 7, 2024 15:04

Merge branch 'MarquezProject:main' into api/add_job_tags

4065cb5

update based on PR feedback.

4c18168

Signed-off-by: sharpd <[email protected]>

correct db field error

10703c1

Signed-off-by: sharpd <[email protected]>

wslulciuc approved these changes May 7, 2024

View reviewed changes

Merge branch 'main' into api/add_job_tags

bf6621e

wslulciuc added this to the 0.47.0 milestone May 7, 2024

wslulciuc enabled auto-merge (squash) May 7, 2024 15:55

wslulciuc merged commit 00b6d35 into MarquezProject:main May 7, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add job tagging to API #2774

Add job tagging to API #2774

davidsharp7 commented Mar 17, 2024 •

edited

Loading

netlify bot commented Mar 17, 2024 •

edited

Loading

codecov bot commented Mar 19, 2024 •

edited

Loading

phixMe Apr 23, 2024

davidjgoss Apr 24, 2024

davidsharp7 Apr 24, 2024 •

edited

Loading

davidjgoss Apr 23, 2024

davidsharp7 Apr 25, 2024 •

edited

Loading

wslulciuc May 7, 2024

davidjgoss Apr 23, 2024

davidjgoss Apr 23, 2024

davidjgoss Apr 23, 2024

davidjgoss left a comment

davidsharp7 commented Apr 25, 2024 •

edited

Loading

wslulciuc May 7, 2024

wslulciuc left a comment

Add job tagging to API #2774

Add job tagging to API #2774

Conversation

davidsharp7 commented Mar 17, 2024 • edited Loading

Problem

Solution

Checklist

netlify bot commented Mar 17, 2024 • edited Loading

✅ Deploy Preview for peppy-sprite-186812 canceled.

codecov bot commented Mar 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsharp7 Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsharp7 Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidjgoss left a comment

Choose a reason for hiding this comment

davidsharp7 commented Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

wslulciuc left a comment

Choose a reason for hiding this comment

davidsharp7 commented Mar 17, 2024 •

edited

Loading

netlify bot commented Mar 17, 2024 •

edited

Loading

codecov bot commented Mar 19, 2024 •

edited

Loading

davidsharp7 Apr 24, 2024 •

edited

Loading

davidsharp7 Apr 25, 2024 •

edited

Loading

davidsharp7 commented Apr 25, 2024 •

edited

Loading