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

deprecate some libutil classes (cleanup) #1047

Merged
merged 10 commits into from Apr 28, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Apr 27, 2017

Two "classes" in libutil aren't used much, depend on json-c, and call oom() internally on malloc failure.

base64_json includes functions for encoding/decoding opaque data blobs to/from json-c objects. It is used by zio.c, iowatcher (lua), and some tests. zio_json_encode() and zio_json_decode() were reworked to use base64 directly. iowatcher now just calls zio_json_decode(). t/kvs/basic.c was reworked to use base64 directly. Finally the kap test, which uses MPI, isn't driven by the test suite, and still uses deprecated Flux API calls, was dropped.

getrusage_json calls getrusage(2) and converts the returned struct to a json-c object. This was called from two places in the broker, one to implement the "cmb.rusage" request hander, and two, to implement thegeneric ".rusage" request handler for modules. I combined the two request handlers and pulled in the function.

@dongahn: on dropping KAP - I know you put a lot of work into this. I was thinking you may wish to bring it back as part of the flux scalability project at some point. It needs some updating, but as we were building it but not running it, I was not comfortable modifying since I might break something and never know it. Better for it to live someplace where it gets run regularly as part of CI.

@garlick garlick force-pushed the garlick:misc_cleanup branch 2 times, most recently from aacb7c7 to 0ed1e90 Apr 27, 2017

garlick added some commits Apr 26, 2017

broker/rusage: refactor rusage request handler
Problem: there are duplicate request handlers for cmb.rusage
and <module>.rusage

Combine broker cmb.rusage and <module>.rusage using an approach
similar to that used for ping.  Pull in the getrusage_json()
function since this is the only user.
libsubprocess/zio: use jansson/base64 directly
Wean zio off of json-c and the libutil/base64_json class.
tests/kap: remove unused test
Problem: kap needs MPI, uses deprecated flux interfaces,
and is not driven by the automated test suite.

Drop it from flux-core for now; it may be appropriate to bring
it back as part of the scalability testing repo at a later date.
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 27, 2017

Oh, nice cleanup!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 27, 2017

Coverage Status

Coverage decreased (-0.02%) to 78.135% when pulling 0ed1e90 on garlick:misc_cleanup into 4937852 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 27, 2017

Codecov Report

Merging #1047 into master will decrease coverage by 0.02%.
The diff coverage is 72.94%.

@@            Coverage Diff             @@
##           master    #1047      +/-   ##
==========================================
- Coverage    77.9%   77.87%   -0.03%     
==========================================
  Files         150      148       -2     
  Lines       25684    25670      -14     
==========================================
- Hits        20008    19991      -17     
- Misses       5676     5679       +3
Impacted Files Coverage Δ
src/common/libutil/shortjson.h 91.48% <ø> (-0.43%) ⬇️
src/cmd/flux-kvs.c 77.05% <ø> (ø) ⬆️
src/broker/broker.c 71.91% <100%> (ø) ⬆️
src/bindings/lua/flux-lua.c 81.53% <100%> (+0.05%) ⬆️
src/broker/modservice.c 79.2% <50%> (-1.53%) ⬇️
src/common/libsubprocess/zio.c 70.84% <60.41%> (-1.33%) ⬇️
src/broker/rusage.c 78.57% <89.28%> (+28.57%) ⬆️
src/common/libflux/request.c 89.04% <0%> (-1.37%) ⬇️
src/common/libflux/message.c 80.95% <0%> (-0.25%) ⬇️
... and 3 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 27, 2017

Coverage Status

Coverage decreased (-0.009%) to 78.146% when pulling 0ed1e90 on garlick:misc_cleanup into 4937852 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Apr 27, 2017

One build failed here

FAIL: t2000-wreck.t 5 - wreckrun: propagates current working directory

I went ahead and restart that one.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 27, 2017

Looks like after these changes Jget_bool() no longer has any users, maybe it could be removed as part of this PR?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Apr 27, 2017

Will do. Yay!

garlick added some commits Apr 27, 2017

cmd/flux-list-instances: drop command
Problem: flux-list-instances no longer works, has no test
coverage, and uses a fairly ad-hoc and fragile method
to find instances.

As discussed in #1034, this command probably should be
replaced with a scheme that uses the system instance
as the point of contact.

Drop the command and its helper function over in the
python bindings.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 28, 2017

Coverage Status

Coverage decreased (-0.03%) to 78.127% when pulling 4a622f0 on garlick:misc_cleanup into 4937852 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 28, 2017

Coverage Status

Coverage decreased (-0.02%) to 78.131% when pulling 4a622f0 on garlick:misc_cleanup into 4937852 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 28, 2017

Small code coverage drop is only due to error conditions, so merging this one...

@grondo grondo merged commit 6ea311e into flux-framework:master Apr 28, 2017

2 of 4 checks passed

codecov/patch 72.94% of diff hit (target 77.9%)
Details
codecov/project 77.87% (-0.03%) compared to 4937852
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 78.131%
Details
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Apr 28, 2017

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

@garlick garlick deleted the garlick:misc_cleanup branch Sep 6, 2017

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.