job-list: split out job information into new files #4575
Conversation
chu11
force-pushed
the
job_list_refactor
branch
from
d908a8b to
e5d3bc5
Compare
|
I thought the idea proposed in #4555 was to split out some of the common R and jobspec "parsing" into a reusable library (i.e. that could eventually be reused by some other facility in flux-core if necessary) Is this just a stepping stone to that goal since here it seems like all that is done so far is to move some code into a new source file? |
|
yeah, this is just moving in that general direction. The main reason I did this is b/c I sort of had the same "move data structures into new files" in two separate PRs, so I felt this refactoring was sort of necessary to avoid more conflicts going forward. |
|
Ok, that seems reasonable. We should indeed try to refactor the jobspec/R parsing into 'struct job' into reusable code with unit tests at some point though. |
Problem: In several locations an extra level of indirection is used to get the 'jsctx' value. Solution: Get the `jsctx` value more directly.
Problem: struct job currently caches two sub-objects of the jobspec in jobspec_job and jobspec_cmd. However, these two sub objects are never used. Solution: Instead store a reference to the jobspec in struct job. Remove the references to the sub objects.
Problem: `struct job` in job_state.h contains `struct list_ctx` from job-list.h. `struct list_ctx` in job-list.h contains `struct job_state_ctx` from `job_state.h`. This circular dependency isn't a problem at the moment, but it can become an issue in the future when future services are added. In addition, job_state.[ch] files are getting long. Solution: Move struct job and job create/destroy functions into their own files. Also move jobspec parsing and R parsing into the new file.
jameshcorbett
force-pushed
the
job_list_refactor
branch
from
e5d3bc5 to
7b32c98
Compare
Codecov Report
@@ Coverage Diff @@
## master #4575 +/- ##
==========================================
- Coverage 83.35% 83.35% -0.01%
==========================================
Files 409 410 +1
Lines 68548 68542 -6
==========================================
- Hits 57140 57130 -10
- Misses 11408 11412 +4
|
This is a general refactoring PR.
I actually did some similar refactoring in #4336
I did some similar refactoring in a (now tabled) PR for #4404
@garlick commented on some potential refactoring in #4555
So thus this refactoring, and why I put it in a separate PR.
I could add some unit tests to capture some worst offending failure cases (e.g. like jobspec that is just a curly brace), but didn't given its limited coverage increase. Some of this is already handled in sharness tests.