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

libjob: Add flux_job_kvs_key() utility function #2068

Merged
merged 1 commit into from Mar 8, 2019

Conversation

@chu11
Copy link
Contributor

commented Mar 8, 2019

Minor cleanup peeled off from #2066, to remove duplicate code from job-manager and libschedutil.

@grondo

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

Good cleanup!

Since you've split the "types" into job_types.h, shouldn't the enum definitions go in job_types.h as well?

I also think it would be slightly cleaner to just put flux_job_kvs_key() into job.c directly, but I can see the rationale for separate module for testing (and the git mv nicely preserved authorship). However, perhaps the prototype could just go into job.h to reduce the number of installed headers? (Just my opinion though)

PR title should be "libjob: Add flux_job_kvs_key" not "libjobutil"

@chu11 chu11 changed the title libjobutil: Add flux_job_kvs_key() utility function libjob: Add flux_job_kvs_key() utility function Mar 8, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

Since you've split the "types" into job_types.h, shouldn't the enum definitions go in job_types.h as well?

I suppose that could go in there too. I sort of followed the pattern of what was done in libflux/types.h, putting that rare typedef that was needed to fix the circular header includes.

I also think it would be slightly cleaner to just put flux_job_kvs_key() into job.c directly,

Good point. I did it mostly b/c I thought there could be future additions in job_util, but perhaps I'm jumping the gun?? Can go either way on this of course.

(and the git mv nicely preserved authorship).

Heh, the code actually come from job-manager, git was the one that decided it was a git mv. So even git knew the code was duplicated and should be cleaned up :-)

@garlick

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

I'd vote for one installed header too, unless there is a practical reason to split it up. We can always do so later if one arises.

@chu11 chu11 force-pushed the chu11:libjobutil_cleanup branch from 3d86ae8 to 07727a3 Mar 8, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

re-pushed, everything is now in job.[ch]. Since no new files, no need to create job_types.h (although this may be brought back when I finish up what's in #2066).

@chu11 chu11 force-pushed the chu11:libjobutil_cleanup branch from 07727a3 to 5dd6a32 Mar 8, 2019
@garlick

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

Thanks! Couple minor nits

  • commit message should drop the trailing "libjob"
  • libjob.la should not need to be added to Makefile.am in job-manager since libjob.lais included in libflux-core.la
@chu11 chu11 force-pushed the chu11:libjobutil_cleanup branch from 5dd6a32 to da25af5 Mar 8, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

re-pushed with nit fixes

@garlick

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

There's still an (unnecessary?) addition of libjob.la to test_ldadd in the job-manager Makefile.am...

Transfer tests from job-manager/tests to libjob/test.

Use function in job-manager and libschedutil and remove duplicate code.
@chu11 chu11 force-pushed the chu11:libjobutil_cleanup branch from da25af5 to ef0dfb1 Mar 8, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

doh! sorry, I forgot about the one for the tests. Re-pushed.

@codecov-io

This comment has been minimized.

Copy link

commented Mar 8, 2019

Codecov Report

Merging #2068 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2068      +/-   ##
==========================================
+ Coverage   80.41%   80.41%   +<.01%     
==========================================
  Files         192      191       -1     
  Lines       30345    30343       -2     
==========================================
- Hits        24401    24400       -1     
+ Misses       5944     5943       -1
Impacted Files Coverage Δ
src/modules/job-manager/event.c 70.37% <100%> (ø) ⬆️
src/common/libschedutil/ops.c 58.92% <100%> (ø) ⬆️
src/common/libschedutil/hello.c 59.25% <100%> (ø) ⬆️
src/common/libjob/job.c 65.9% <100%> (+3.4%) ⬆️
src/modules/job-manager/util.c 84.74% <100%> (-1.2%) ⬇️
src/common/libschedutil/alloc.c 69.64% <100%> (ø) ⬆️
src/modules/barrier/barrier.c 76.55% <0%> (-2.07%) ⬇️
src/cmd/flux-event.c 77.97% <0%> (ø) ⬆️
src/modules/connector-local/local.c 73.62% <0%> (+0.14%) ⬆️
src/common/libflux/message.c 81.51% <0%> (+0.24%) ⬆️
... and 1 more
@garlick

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

Thanks! LGTM!

@garlick garlick merged commit 1a6e9dd into flux-framework:master Mar 8, 2019
4 checks passed
4 checks passed
Mergify — Summary 1 potential rule
Details
codecov/patch 100% of diff hit (target 80.41%)
Details
codecov/project 80.41% (+<.01%) compared to 220dfcd
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
4 participants
You can’t perform that action at this time.