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

convert broker from json-c to jansson (cleanup) #1050

Merged
merged 12 commits into from May 1, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Apr 29, 2017

This PR is more cleanup. It converts the remaining broker C modules that use json-c to jansson.

@grondo it may be good for you to have a quick look over the exec.c commit proposed here. It's passing tests on my c9 image, but I did need to change it more than I wanted to make the error paths more clear (to me) as I worked through it. If this causes grief in your imp test branch, we can always skip this commit here and try again later.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 29, 2017

Codecov Report

Merging #1050 into master will decrease coverage by 0.24%.
The diff coverage is 60.18%.

@@            Coverage Diff             @@
##           master    #1050      +/-   ##
==========================================
- Coverage   77.93%   77.68%   -0.25%     
==========================================
  Files         148      148              
  Lines       25699    25739      +40     
==========================================
- Hits        20028    19996      -32     
- Misses       5671     5743      +72
Impacted Files Coverage Δ
src/broker/modservice.c 79.2% <ø> (ø) ⬆️
src/broker/heartbeat.c 88.05% <ø> (ø) ⬆️
src/broker/module.c 82.15% <ø> (ø) ⬆️
src/broker/service.c 98.24% <ø> (ø) ⬆️
src/broker/runlevel.c 83.94% <100%> (-0.12%) ⬇️
src/broker/exec.c 66.66% <57.14%> (-20.1%) ⬇️
src/broker/overlay.c 66.77% <58.33%> (-0.86%) ⬇️
src/broker/ping.c 63.76% <65.62%> (-2.27%) ⬇️
src/broker/broker.c 71.83% <66.66%> (-0.09%) ⬇️
src/broker/rusage.c 61.76% <71.42%> (-16.81%) ⬇️
... and 4 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 29, 2017

Coverage Status

Coverage decreased (-0.2%) to 78.016% when pulling 5d3cbe9 on garlick:broker_json into e83fd0e on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Apr 29, 2017

Something's likely gone wrong here given the coverage drop - I'll have to look into this on monday.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 29, 2017

It is difficult to review the broker/exec commit because it is a conversion to jansson and a rewrite of the code. Ideally it would be split into an update to jansson, then the rewrite/refactor, (which seems to be good afaict), as this would be easier to review and follow in history. However, In this case since the work is already done and tests are passing I'd say go ahead and keep the changes. (But perhaps rename the commit message to "broker/exec: rewrite and update to jansson" with an actual commit body that mentions the code was refactored for better error handling and code clarity. (Could some of the other commits use bodies as well?)

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Apr 29, 2017

Looking at the code coverage changes, I don't think there is a problem here per se, but the update to jansson does seem to add a lot more unchecked error conditions, which seems to account for majority of the misses I'm seeing.

@garlick garlick force-pushed the garlick:broker_json branch from 5d3cbe9 to 1c41f2e Apr 29, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Apr 29, 2017

Sorry about the big, unreviewable diff. I didn't think about breaking it up while I was doing it, but I should have.

I just forced a push with a better comment on that one, FWIW.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 29, 2017

Coverage Status

Coverage decreased (-0.2%) to 77.944% when pulling 1c41f2e on garlick:broker_json into e83fd0e on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 1, 2017

I'll go ahead and split that commit into refactoring + json changes. Don't merge this as is please.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 1, 2017

garlick added some commits May 1, 2017

broker/exec: store local-uri value, not attr hash
Problem: exec service stores attribute hash but only
uses it once to obtain the 'local-uri' value.

In exec_initialize(), obtain the local-uri value and
cache that instead of the attribute hash.
broker/exec: factor out prepare_exit_payload
In preparation for jansson conversion and more error
handling for JSON-related failures, split the response
payload creation out of child_exit_handler() into
prepare_exit_payload().
broker/exec: factor out prepare_io_payload
In preparation for jansson conversion and more error
handling for JSON-related failures, split the response
payload creation out of child_io_cb() into
prepare_io_payload().

child_io_cb() was renamed from subprocess_io_cb to avoid
namespace confusion with the subprocess class.
broker/exec: factor out write_to_child
In preparation for jansson conversion and more error
handling for JSON-related failures, split the zio-decode
and subprocess write calls from write_request_cb().
Alter request payload parsing logic slightly to make
it easier to drop-in requestf.
broker/exec: parse cmb.exec.signal request once
Problem: flux_request_decodef() is called twice, once
for required key, once for optional key.

Call it once and use "s?:i" in the format specifier
to indicate that the signal number is optional.
broker/exec: factor out prepare_subprocess
In preparation for jansson conversion and more error
handling for JSON-related failures, split the subprocess
allocation and configuration out from the cmb.exec handler.

@garlick garlick force-pushed the garlick:broker_json branch from 1c41f2e to 80c232c May 1, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 1, 2017

I've split up the commits to the exec service into several refactoring commits and one final jansson conversion commit.

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 1, 2017

Coverage Status

Coverage decreased (-0.2%) to 77.943% when pulling 80c232c on garlick:broker_json into e83fd0e on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 1, 2017

Oh, thanks for all the work. Nice cleanup!

One question that isn't clear from jansson docs, does json_decref handle a NULL argument?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented May 1, 2017

Yes it's an inline so easy to check

static JSON_INLINE
void json_decref(json_t *json)
{
    if(json && json->refcount != (size_t)-1 && --json->refcount == 0)
        json_delete(json);
}
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented May 1, 2017

Yes it's an inline so easy to check

Ok, sorry I didn't do that!

@grondo grondo merged commit f1b9b45 into flux-framework:master May 1, 2017

2 of 4 checks passed

codecov/patch 60.18% of diff hit (target 77.93%)
Details
codecov/project 77.68% (-0.25%) compared to e83fd0e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.2%) to 77.943%
Details

@garlick garlick deleted the garlick:broker_json branch May 1, 2017

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

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.