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

sim: fix emulator after introduction of new submit RPC #304

Merged
merged 4 commits into from
Apr 1, 2018

Conversation

SteVwonder
Copy link
Member

The main problem was a pre-mature destruction of the flux_future. The unpacked values were later accessed in the packing of the wreck event. While making changes in the simulator, went ahead and added some additional error checking/messaging and refactored to take advantage of the new job.submit-nocreate RPC.

Fixes: #303

@SteVwonder SteVwonder requested review from grondo and dongahn April 1, 2018 03:38
@SteVwonder
Copy link
Member Author

@grondo: just want to confirm that I interpreted the job.submit-nocreate control flow correctly. It does not change the state of the job in the KVS to "submitted", correct?

@garlick
Copy link
Member

garlick commented Apr 1, 2018

(Sorry if I introduced the premature future destruction bug when updating the KVS API!)

@codecov-io
Copy link

codecov-io commented Apr 1, 2018

Codecov Report

Merging #304 into master will increase coverage by 6.37%.
The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
+ Coverage   67.74%   74.11%   +6.37%     
==========================================
  Files          46       49       +3     
  Lines        8692     9287     +595     
==========================================
+ Hits         5888     6883     +995     
+ Misses       2804     2404     -400
Impacted Files Coverage Δ
simulator/submitsrv.c 77.5% <70%> (ø)
simulator/simsrv.c 76.64% <0%> (ø)
simulator/sim_execsrv.c 84.77% <0%> (ø)
resrc/resrc.c 83.05% <0%> (+1.27%) ⬆️
sched/sched_fcfs.c 94.31% <0%> (+5.68%) ⬆️
src/common/libutil/shortjansson.h 88.33% <0%> (+10.36%) ⬆️
sched/sched.c 73.95% <0%> (+12.25%) ⬆️
sched/rs2rank.c 94.82% <0%> (+14.65%) ⬆️
sched/rsreader.c 96.52% <0%> (+20.13%) ⬆️
... and 2 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 02b17e9...fa44e05. Read the comment docs.

@coveralls
Copy link

coveralls commented Apr 1, 2018

Coverage Status

Coverage increased (+6.9%) to 75.851% when pulling fa44e05 on SteVwonder:fix-emulator-new-submit into 02b17e9 on flux-framework:master.

@dongahn
Copy link
Member

dongahn commented Apr 1, 2018

Thanks @SteVwonder. LGTM!

I will merge this after flux-core 1409 gets in.

@garlick
Copy link
Member

garlick commented Apr 1, 2018

flux-framework/flux-core#1409 went in just now, so kicking off tests again to run against the new master.

@grondo
Copy link
Contributor

grondo commented Apr 1, 2018

@grondo: just want to confirm that I interpreted the job.submit-nocreate control flow correctly. It does not change the state of the job in the KVS to "submitted", correct?

@SteVwonder, oops I missed your question yesterday, sorry. The job.submit-nocreate probably should have updated the job state to submitted for symmetry with job.submit. However, now that you point it out, I left that out which was probably an oversight. Sorry!

We can fix that up later if it makes more sense. (Current behavior might be confusing because it emits the submitted event, but doesn't change the state)

@grondo
Copy link
Contributor

grondo commented Apr 1, 2018

Opened flux-framework/flux-core#1410 on the question of submitted state for job.submit-nocreate RPC.

@garlick
Copy link
Member

garlick commented Apr 1, 2018

Thanks! Merging.

@garlick garlick merged commit cc03941 into flux-framework:master Apr 1, 2018
@SteVwonder SteVwonder deleted the fix-emulator-new-submit branch April 2, 2018 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants