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

avoid deprecated KVS functions, minor simulator cleanup #272

Merged
merged 8 commits into from Oct 24, 2017

Conversation

Projects
None yet
5 participants
@garlick
Copy link
Member

garlick commented Oct 18, 2017

This PR was peeled off of #271. It migrates sched off of functions that now cause "deprecated" warnings from the compiler. In the process, I did some minor refactoring and error handling cleanup in the simulator code surrounding KVS access to job data.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 18, 2017

Coverage Status

Coverage increased (+0.3%) to 74.966% when pulling c181e29 on garlick:sim_cleanup into be23ee7 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 18, 2017

Codecov Report

Merging #272 into master will increase coverage by 0.2%.
The diff coverage is 83.78%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #272     +/-   ##
=========================================
+ Coverage    74.8%   75.01%   +0.2%     
=========================================
  Files          34       34             
  Lines        7332     7337      +5     
=========================================
+ Hits         5485     5504     +19     
+ Misses       1847     1833     -14
Impacted Files Coverage Δ
simulator/submitsrv.c 80.97% <100%> (-0.11%) ⬇️
simulator/sim_execsrv.c 85.49% <83.33%> (-0.97%) ⬇️
simulator/simulator.c 90.75% <83.67%> (+7.06%) ⬆️

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 44c2e3e...52491ec. Read the comment docs.

@garlick garlick force-pushed the garlick:sim_cleanup branch from c181e29 to 914d742 Oct 19, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 19, 2017

Coverage Status

Coverage increased (+0.3%) to 74.966% when pulling 914d742 on garlick:sim_cleanup into a5d83bb on flux-framework:master.

@garlick garlick requested a review from SteVwonder Oct 19, 2017

@SteVwonder

This comment has been minimized.

Copy link
Member

SteVwonder commented Oct 23, 2017

LGTM. Thanks for the cleanup and modernization, @garlick! Can you do a rebase, and then I'll push the button?

@garlick garlick force-pushed the garlick:sim_cleanup branch from 914d742 to 52491ec Oct 23, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 23, 2017

NP! just forced a push after a rebase on current master.

garlick added some commits Oct 18, 2017

simulator: [cleanup] drop else chain in put_job_in_kvs()
Eliminate chain of 'else' clauses in put_job_in_kvs()
error handling for KVS functions.  The "goto on error"
idiom makes them redundant.
simulator: [cleanup] drop recovery from put_job_in_kvs()
If the KVS commit fails, there is no need to restore
the directory pointer from the last known good one.
Nothing is changed unless the commit succeeds.
simulator: [cleanup] reduce logging in put_job_in_kvs()
In the unlikely event that something goes wrong updating state
in the KVS, it's probably sufficient to know that it happened,
and not that useful to know what key.
simulator: [cleanup] rework get_job_from_kvs() helpers
As part of migrating off of deprecated KVS functions -

Rename helper functions that load values from the KVS
so they will not be confused with similar core functions.
Reduce the "typed" helper functions to two: one for
strings, and one for scalars.

Check function return values for error and reduce logging
to a single failure message if something goes wrong,
mirroring the error handling in put_job_in_kvs().
simulator: [cleanup] use txn in put_job_in_kvs
Switch put_job_in_kvs() to an explicit KVS transaction
from the deprecated flux_kvsdir_pack() and
flux_kvs_commit_anon() functions.
simulator: [cleanup] put_job_in_kvs sets initial state
Set initial job state as part of put_job_in_kvs() so
it can continue to be done in the same KVS transaction
as the rest of the KVS setup for the job, as it was before
migration to an explicit transaction.
simulator: [cleanup] modernize job_kvsdir()
Factor job_kvspath(), which makes an RPC to the job
module, from job_kvsdir().  Then update KVS API
usage in job_kvsdir().
simulator: [cleanup] add set_job_timestamps()
Add set_job_timestamps(), a function to update any
combination of job timestamp keys, and use it from
update_job_state() and complete_job().

This co-locates the functions that read/update the
KVS job schema in simulator.c, and migrates off newly
deprecated KVS interfaces.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 23, 2017

Coverage Status

Coverage increased (+0.2%) to 77.303% when pulling 52491ec on garlick:sim_cleanup into 44c2e3e on flux-framework:master.

@lipari lipari merged commit a55889f into flux-framework:master Oct 24, 2017

4 checks passed

codecov/patch 83.78% of diff hit (target 74.8%)
Details
codecov/project 75.01% (+0.2%) compared to 44c2e3e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 77.303%
Details
@lipari

This comment has been minimized.

Copy link
Contributor

lipari commented Oct 24, 2017

Many thanks, @garlick !

@grondo grondo referenced this pull request May 11, 2018

Closed

Need 0.5.0 Release #340

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.