Skip to content
This repository has been archived by the owner on May 24, 2022. It is now read-only.

Add ability to list jobs #213

Merged
merged 1 commit into from
Jan 21, 2021
Merged

Add ability to list jobs #213

merged 1 commit into from
Jan 21, 2021

Conversation

rojer
Copy link
Contributor

@rojer rojer commented Jan 19, 2021

With filtering by state and tags.

ListJobs call is added to the Storage interface and implemented for both
in-memory and RDBMS storage plugins. RDBMS implementation requires
database schema changes to allow efficient lookups:

  • "state" column is added to the jobs table
  • "job_tags" table is added to map tags to job_id

Database migration script is provided to create and populate the new
columns. Going forward, they will be maintained by the server.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 19, 2021
@rojer rojer force-pushed the list_jobs branch 2 times, most recently from 7e18920 to ee4597a Compare January 19, 2021 12:56
@codecov
Copy link

codecov bot commented Jan 19, 2021

Codecov Report

Merging #213 (e1b68f1) into master (1a15df7) will increase coverage by 0.50%.
The diff coverage is 70.40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #213      +/-   ##
==========================================
+ Coverage   61.35%   61.86%   +0.50%     
==========================================
  Files          84       88       +4     
  Lines        4117     4345     +228     
==========================================
+ Hits         2526     2688     +162     
- Misses       1270     1324      +54     
- Partials      321      333      +12     
Flag Coverage Δ
integration 56.71% <66.97%> (+0.43%) ⬆️
integration_storage 100.00% <ø> (ø)
unittests 49.74% <13.15%> (-1.73%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/api/api.go 58.13% <0.00%> (-8.83%) ⬇️
pkg/api/event.go 62.50% <0.00%> (-8.93%) ⬇️
pkg/api/response.go 0.00% <0.00%> (ø)
pkg/jobmanager/jobmanager.go 54.58% <0.00%> (-0.51%) ⬇️
pkg/jobmanager/list.go 0.00% <0.00%> (ø)
pkg/jobmanager/status.go 0.00% <0.00%> (ø)
pkg/storage/events.go 50.00% <ø> (ø)
pkg/storage/storage.go 33.33% <ø> (ø)
pkg/transport/http/http.go 0.00% <0.00%> (ø)
plugins/storage/rdbms/request.go 59.32% <25.00%> (-6.72%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a15df7...e1b68f1. Read the comment docs.

Copy link
Contributor

@tfg13 tfg13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly nits, 2 main things:

  • I am really scared by the SQL stuff, see inline
  • Will the list API call return all jobs that ever ran and match the filters? I.e. will this return 100000s of jobs potentially? Should it? Do we know approx how many jobs until it OOMs or the DB aborts the query because it is too large?

pkg/job/job.go Outdated Show resolved Hide resolved
pkg/storage/job.go Outdated Show resolved Hide resolved
plugins/storage/rdbms/events.go Show resolved Hide resolved
plugins/storage/rdbms/list_jobs.go Show resolved Hide resolved
plugins/storage/rdbms/list_jobs.go Show resolved Hide resolved
tests/integ/common/storage.go Outdated Show resolved Hide resolved
@rojer
Copy link
Contributor Author

rojer commented Jan 20, 2021

forgot to respond to this one, so here goes:

Mostly nits, 2 main things:

  • I am really scared by the SQL stuff, see inline

addressed

  • Will the list API call return all jobs that ever ran and match the filters? I.e. will this return 100000s of jobs potentially? Should it? Do we know approx how many jobs until it OOMs or the DB aborts the query because it is too large?

so, yes, the API has no pagination support at the moment. yes, unfiltered query will return all 100000 or so jobs, but for now it's just 100K integers, so it's not so bad. the first real use case for this will be to find all the paused jobs at the instance startup, of which there will never be more than a single instance can handle anyway, so it's fine.
i can add an explicit TODO to add pagination to API and put a hard limit on the number of returned results.

@rojer rojer force-pushed the list_jobs branch 2 times, most recently from 714560d to 14bc45e Compare January 20, 2021 16:01
@rojer
Copy link
Contributor Author

rojer commented Jan 20, 2021

rebased

With filtering by state and tags.

ListJobs call is added to the Storage interface and implemented for both
in-memory and RDBMS storage plugins. RDBMS implementation requires
database schema changes to allow efficient lookups:
 * "state" column is added to the jobs table
 * "job_tags" table is added to map tags to job_id

Database migration script is provided to create and populate the new
columns. Going forward, they will be maintained by the server.
@rojer
Copy link
Contributor Author

rojer commented Jan 21, 2021

addressed 2 comments i somehow missed before

@rojer rojer merged commit 2885d99 into master Jan 21, 2021
@rojer rojer deleted the list_jobs branch January 21, 2021 12:13
rojer9-fb added a commit that referenced this pull request Feb 22, 2021
With filtering by state and tags.

ListJobs call is added to the Storage interface and implemented for both
in-memory and RDBMS storage plugins. RDBMS implementation requires
database schema changes to allow efficient lookups:
 * "state" column is added to the jobs table
 * "job_tags" table is added to map tags to job_id

Database migration script is provided to create and populate the new
columns. Going forward, they will be maintained by the server.

This is take 2, first was #213 but was reverted due to unrelated issue.
There are no substantial changes here.
llogen pushed a commit to 9elements/contest that referenced this pull request Aug 4, 2021
With filtering by state and tags.

ListJobs call is added to the Storage interface and implemented for both
in-memory and RDBMS storage plugins. RDBMS implementation requires
database schema changes to allow efficient lookups:
 * "state" column is added to the jobs table
 * "job_tags" table is added to map tags to job_id

Database migration script is provided to create and populate the new
columns. Going forward, they will be maintained by the server.

This is take 2, first was facebookarchive#213 but was reverted due to unrelated issue.
There are no substantial changes here.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants