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

Issue 495 - jansson style rpc calls #1002

Merged
merged 23 commits into from Mar 15, 2017

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Mar 13, 2017

Various refactoring to update rpc calls from json-c to jansson style. Such as calling flux_rpcf() over flux_rpc().

There are more that can be done, but I limited this PR to only those rpcs that dealt with simple int, string, etc. types. Dealing with more complex objects/arrays require using the jansson library to create/manipulate said objects/arrays instead of the json-c library, and that's refactoring/cleanup for another day.

Most fixes should be self explanatory hopefully. The noteable exception is 37be059. The jansson style functions are pickier on typing. So a "1" (in quotes) is a string and not an integer, so I updated flux-cron to send the right type.

@chu11 chu11 force-pushed the chu11:issue495-jansson branch from f7dbc8c to 1b2a015 Mar 13, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 13, 2017

Coverage Status

Coverage decreased (-0.03%) to 76.445% when pulling 1b2a015 on chu11:issue495-jansson into 44fdb7a on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 13, 2017

Codecov Report

Merging #1002 into master will decrease coverage by <.01%.
The diff coverage is 79.36%.

@@            Coverage Diff             @@
##           master    #1002      +/-   ##
==========================================
- Coverage   76.77%   76.76%   -0.01%     
==========================================
  Files         151      151              
  Lines       25954    25724     -230     
==========================================
- Hits        19925    19748     -177     
+ Misses       6029     5976      -53
Impacted Files Coverage Δ
src/modules/kvs/proto.c 75% <ø> (+0.88%)
src/modules/content-sqlite/content-sqlite.c 54.54% <100%> (-0.31%)
src/cmd/builtin/heaptrace.c 57.44% <100%> (-1.74%)
src/broker/shutdown.c 89.33% <100%> (+0.82%)
src/broker/log.c 61.09% <100%> (-0.8%)
src/common/libflux/heartbeat.c 81.81% <100%> (-0.8%)
src/cmd/builtin/proxy.c 72.81% <100%> (+0.17%)
src/modules/kvs/libkvs.c 75.38% <100%> (+0.03%)
src/connectors/shmem/shmem.c 75% <100%> (-0.66%)
src/connectors/ssh/ssh.c 85.23% <100%> (-0.28%)
... and 19 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 0071f76...5dda825. Read the comment docs.

@chu11 chu11 force-pushed the chu11:issue495-jansson branch from 1b2a015 to fdf02ab Mar 13, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 13, 2017

Coverage Status

Coverage decreased (-0.02%) to 76.449% when pulling 1b2a015 on chu11:issue495-jansson into 44fdb7a on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 13, 2017

Oh, there is one other thing that may be viewed as quirky. The flux_rmmod_json_decode() and flux_rmmod_json_encode() functions only encode/decode a single name string, so I refactored them out. Unsure if they would be desired to be left in for some general API abstraction desires.

I didn't remove the functions outright b/c flux-sched uses flux_rmmod_json_decode() for the moment.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 13, 2017

Coverage Status

Coverage decreased (-0.02%) to 76.451% when pulling fdf02ab on chu11:issue495-jansson into 44fdb7a on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Mar 13, 2017

One suggestion is to remove the #include shortjson.h or #include json.h wherever possible.

As noted in 0de293f, the internal json-c library is "re-namespaced" using the preprocessor to put a json_c_ prefix on all functions. The redefinition covers the source files used to build libjson-c.a, and any Flux C modules that include the above files.

There might not be a problem as long as overlapping names are avoided, as is probably the case with all your changes. However, just to be safe since json code could be introduced in the future and the preferred library is jansson, if you've weaned C module off of json-c, it is best to drop the include.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 14, 2017

@garlick Ahh, I had missed that. I think I even found a few places we could remove jansson.h as well.

While going through cleanup of shortjson.h, I realized that I could also cleanup usage of flux_event_encode(). That is one I had missed earlier. I'll try to add more cleanup with that as well.

@chu11 chu11 force-pushed the chu11:issue495-jansson branch 2 times, most recently from 9ac78f4 to c4887cb Mar 14, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 14, 2017

Reworked the PR. Added some additional cleanup using flux_event_encodef() and flux_event_decodef(). Removed shortjson.h where possible.

Also elected to not refactor out use of flux_rmmod_json_decode() and flux_rmmod_json_encode(). I think it's better to refactor those in another way.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 14, 2017

Coverage Status

Coverage increased (+0.02%) to 76.496% when pulling c4887cb on chu11:issue495-jansson into 44fdb7a on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 14, 2017

Coverage Status

Coverage increased (+0.01%) to 76.485% when pulling c4887cb on chu11:issue495-jansson into 44fdb7a on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 14, 2017

Looking through code coverage, unsurprisingly lots of error paths missed.

It appears no test coverage of ssh connector exists b/c it missed everything there. Lots of live module misses, I suspect from prior removal of topo & up in #960

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Mar 14, 2017

Looking through code coverage, unsurprisingly lots of error paths missed.

Yeah, usually it is difficult to test error paths. As long as the project coverage doesn't drop appreciably, I take the code coverage checks as advisory.

It appears no test coverage of ssh connector exists b/c it missed everything there

Hm, that is concerning ... on master there is good coverage of ssh connector

https://codecov.io/gh/flux-framework/flux-core/src/master/src/connectors/ssh/ssh.c

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Mar 14, 2017

Note in #638 the suggestion that we remove the live module. We need to start from scratch on that one. Want to just remove it here instead of refactoring?

(Sorry I didn't just do it earlier and save you the extra work!)

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 14, 2017

@grondo Ahh, I spoke too soon. It appears op_event_subscribe and op_event_unsubscribe are not covered in the ssh connector, which are the only two places I cleaned up in the ssh connector. Let me see if some tests can be done to done to correct that.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 14, 2017

@garlick Ahh, wasn't aware of that issue. I'll just remove it in another PR and rebase here.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Mar 14, 2017

@chu11, cool good find!

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 14, 2017

Just sent PR #1003 and #1004 to address the poor coverage on the live module and ssh connector. Once those are in, can rebase and hopefully the coverage will be a lot better here.

chu11 added some commits Mar 7, 2017

broker/sequence: Refactor rpc usage
Refactor callbacks for seq.fetch, seq.set, and seq.destroy to
use jansson functions flux_request_decodef() & flux_respondf() instead
of flux_request_decode() & flux_respond().  Refactor out
sequence_request_handler() function.
modules/kvs: Refactor rpc calls
Refactor kvs.getroot and kvs.sync to use jansson style functions
(i.e. flux_respondf() instead of flux_respond(), flux_rpc_getf()
instead of flux_rpc_get(), and flux_request_decodef() instead of
flux_request_decode().
modules/kvs: Remove unused proto functions
Remove kp_rgetroot_enc() and kp_rgetroot_dec() which are no longer
used.
cmd/hwloc: Refactor to use jansson style
Refactor resource-hwloc.reload usage to use jansson style.
In particular, use flux_rpcf_multi() over flux_rpc_multi().
broker/exec: Refactor to jansson style functions
Refactor cmb.exec.signal to use jansson style functions.  In
particular, use flux_request_decodef() over flux_request_decode().
broker/hello: Refactor rpc usage
Refactor hello.join request/response to use jansson style functions.
Including using flux_request_decodef() over flux_request_decode() and
using flux_rpcf() over flux_rpc().
broker/log: Refactor rpc usage
Refactor to use jansson style functions for log.clear and log.dmesg,
including using flux_request_decodef() over flux_request_decode()
and flux_respondf() over flux_respond().
broker/content-cache: Refactor rpc usage
Refactor content.backing callback to use flux_request_decodef() over
flux_request_decode().

chu11 added some commits Mar 9, 2017

modules/content-sqlite: Refactor rpc usage
Refactor content.backing to use flux_rpcf() over flux_rpc().
cmd/proxy: Refactor rpc usage
Refactor sub_request to use flux_request_decodef() over
flux_request_decode().
connectors/ssh: Refactor rpc usage
Refactor local.sub and local.unsub to use flux_rpcf() over flux_rpc().
modules/cron: Refactor rpc usage
In cron.sync, cron.delete, cron.stop, cron.start callbacks and
next_cronid(), refactor rpc usage to jansson style functions.  Replace
calls of flux_request_decode() to flux_request_decodef(), flux_respond()
with flux_respondf(), flux_rpcf() with flux_rpc_getf(), and
flux_rpc() with flux_rpcf().
cmd/flux-cron: Send proper types in rpc
Adjust code to send proper types in rpc, i.e.

Send "id" : 1 instead of "id : "1", latter is a string and not
an int.
broker/broker: Refactor rpc usage
Refactor cmb.reparent, cmb.panic, cmb.sub, and cmb.unsub callbacks to use
jansson functions.  In particular, call flux_request_decodef() instead of
flux_request_decode().
connectors/shmem: Refactor rpc usage
Refactor cmb.sub and cmb.unsub to use flux_rpcf() over flux_rpc().
cmd/heaptrace: Refactor rpc usage
Refactor to use flux_rpcf() over flux_rpc().
broker/content-cache: Refactor rpc usage
Use flux_respondf() over flux_respond() in content.stats.get callback.
broker/modservice: Refactor rpc usage
Use flux_respondf() over flux_respond() in stats.get callback.
modules/resource-hwloc: Refactor rpc usage
Refactor to use jansson style functions in resource-hwloc.reload
and resource-hwloc.topo callbacks.  In particular, replace
flux_request_decode() with flux_request_decodef() and flux_respond()
with flux_respondf().
broker/shutdown: Refactor rpc usage
Refactor rpc usage to jansson style.  Replace usage of flux_event_decode()
with flux_event_decodef() and flux_event_encode() with flux_event_encodef().
modules/wreck: Refactor event usage
Refactor event usage to use jansson style functions by calling flux_event_encodef()
over flux_event_encode() when appropriate.
modules/libjsc: Refactor event usage
Refactor to use jansson style event functions.  Use flux_event_decodef()
over flux_event_decode() and flux_event_encodef() over flux_event_encode()
appropriately.
libflux/heartbeat: Refactor event usage
Refactor to use jansson style event functions.  Use flux_event_decodef()
over flux_event_decode() and flux_event_encodef() over flux_event_encode()
appropriately.

@chu11 chu11 force-pushed the chu11:issue495-jansson branch from c4887cb to 5dda825 Mar 14, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 15, 2017

Coverage Status

Coverage decreased (-0.007%) to 77.044% when pulling 5dda825 on chu11:issue495-jansson into 0071f76 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Mar 15, 2017

Looks good, nice cleanup!

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Mar 15, 2017

Well done!

@garlick garlick merged commit 5ca1767 into flux-framework:master Mar 15, 2017

4 checks passed

codecov/patch 79.36% of diff hit (target 76.77%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +2.59% compared to 0071f76
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.007%) to 77.044%
Details

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

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.