Skip to content
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

job-info: read job data in bulk #2115

Merged
merged 8 commits into from Apr 5, 2019

Conversation

@chu11
Copy link
Contributor

commented Apr 5, 2019

This last PR from #2107 supports reading multiple information from a job in bulk via a single RPC. It at first does two RPCs, a guest access check & then information retrieval. Then later commits collapse both RPCs into a single composite RPC.

In addition, some cleanup in libjob tests. Also added job info call to valgrind test.

chu11 added 8 commits Apr 5, 2019
Refactor lookup_key() to return future.
Support the ability to specify multiple pieces of job info (e.g.
eventlog, jobspec, J, R) in a request and get all of them back in
one request.  Adjust RPC for request by sending json array of keys
desired.
Allow multiple info requests to be specified at one time in
'flux job info'.
Refactor the guest access check into the primary info
lookup, so that it can be done in parallel to the rest of the
job info lookup.
As consequence to combination of guest access check and lookup of in
a single composite future, cleanup lookup functions.  Various functions
no longer need to return a future or take a continuation as an argument.
Eliminate the need for lookup_ctx_set_future().
@codecov-io

This comment has been minimized.

Copy link

commented Apr 5, 2019

Codecov Report

Merging #2115 into master will decrease coverage by 0.04%.
The diff coverage is 72%.

@@            Coverage Diff             @@
##           master    #2115      +/-   ##
==========================================
- Coverage   80.27%   80.22%   -0.05%     
==========================================
  Files         201      201              
  Lines       31663    31712      +49     
==========================================
+ Hits        25417    25441      +24     
- Misses       6246     6271      +25
Impacted Files Coverage Δ
src/modules/job-info/lookup.c 64.28% <69.51%> (-4.04%) ⬇️
src/cmd/flux-job.c 84.27% <83.33%> (-0.36%) ⬇️
src/modules/connector-local/local.c 73.62% <0%> (-1.04%) ⬇️
src/broker/modservice.c 78.84% <0%> (-0.97%) ⬇️
src/broker/module.c 82.74% <0%> (+0.26%) ⬆️
src/common/libflux/mrpc.c 88.93% <0%> (+1.18%) ⬆️
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

diff coverage could be eeked out a bit better if I write some specialized "bad rpc EPROTO" kinda tests. But perhaps not worth it?

@garlick

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

LGTM! Up to you if you feel those tests are worth it.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

nah, I don't think it's worth it. We rarely do those EPROTO tests.

@garlick

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Thanks!

@garlick garlick merged commit cc3c67f into flux-framework:master Apr 5, 2019
3 of 4 checks passed
3 of 4 checks passed
codecov/patch 72% of diff hit (target 80.27%)
Details
Mergify — Summary 1 potential rule
Details
codecov/project 80.22% (-0.05%) compared to 566f39b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.