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 - misc cleanup #1001

Merged
merged 11 commits into from Mar 11, 2017

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Mar 10, 2017

While doing a lot of conversion to jansson-style functions in #495, fixed of variety of cleanup things along the way. Decided to split them all off onto another pull request, before sending over the beefier changes.

Most fixes are obvious, the notable exception may be 57a29a9. It was necessary b/c jansson is pickier on types. (i.e. "1" is a string while 1 is an int). My lua is weak, would appreciate look to make sure toboolean local function is a smart solution?

@chu11 chu11 force-pushed the chu11:issue495-jansson-cleanup branch from 99b1ea7 to 0aaa0ef Mar 10, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 10, 2017

In hindsight, 57a29a9 probably shouldn't have been split off into this "cleanup" set of patches. Re-pushed without it, I'll add that to the beefier PR later on.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Mar 10, 2017

Just took a spin through these and looks like good cleanup to me!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 10, 2017

Coverage Status

Coverage decreased (-0.03%) to 76.418% when pulling 0aaa0ef on chu11:issue495-jansson-cleanup into fdc7099 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 10, 2017

Codecov Report

Merging #1001 into master will increase coverage by <.01%.
The diff coverage is 46.66%.

@@            Coverage Diff             @@
##           master    #1001      +/-   ##
==========================================
+ Coverage   76.17%   76.18%   +<.01%     
==========================================
  Files         153      153              
  Lines       26545    26556      +11     
==========================================
+ Hits        20220    20231      +11     
  Misses       6325     6325
Impacted Files Coverage Δ
src/cmd/builtin/heaptrace.c 59.18% <ø> (ø)
src/modules/live/live.c 51.34% <0%> (ø)
src/cmd/builtin/hwloc.c 82.63% <100%> (ø)
src/broker/sequence.c 84.4% <100%> (ø)
src/modules/resource-hwloc/resource.c 71.82% <37.5%> (-1.07%)
src/broker/exec.c 84.72% <50%> (-1.71%)
src/common/libflux/heartbeat.c 82.6% <0%> (-4.35%)
src/common/libflux/request.c 88.15% <0%> (-1.32%)
src/common/libflux/response.c 84.16% <0%> (-0.84%)
src/modules/kvs/kvs.c 80.29% <0%> (-0.37%)
... and 5 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 aa5d395...84cc37b. Read the comment docs.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 10, 2017

Coverage Status

Coverage decreased (-0.03%) to 76.41% when pulling 0aaa0ef on chu11:issue495-jansson-cleanup into fdc7099 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 10, 2017

looks like most of the code coverage misses are in error handling. Lines in heaptrace were missed, I guess there are no tests for that or something. I'll rebase and push on new master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Mar 10, 2017

Sounds good, I'll merge once that passes travis

chu11 added some commits Mar 7, 2017

broker/sequence: Fix branch logic error
Fix incorrect branch logic.  In prior code, if seq_cmp_and_set()
succeeds, the first half of the "||" branch logic fails, leading to
a subsequent call to seq_set().  Instead, we wish to call
seq_cmp_and_set() or seq_set() only once, based on logic conditions.
cmd/hwloc: Fix variable scope collision
Rename internal block json_str declaration, it collides with
function block's additional json_str declaration.
modules/resource-hwloc: Protocol cleanup
For resource-hwloc.reload response, do not send unnecessary
"empty json array" payload.  Instead do not send a payload.
broker/exec: Update flux_respondf usage
Pass format string to flux_respondf() consistent to how it is
done in rest of Flux.
broker/exec: Fix possible missed error
Properly capture errnum in calls to flux_request_decode() so
it can be sent in response.
broker/exec: Fix errnum fallthrough corner case
In the event no errors occur in cmb.exec.write callback, errnum
was never set to 0, so a error was always returned to the caller.
modules/resource-hwloc: Add error logging
Add error checking/logging in resource-hwloc.reload and
resource-hwloc.topo callbacks.
modules/resource-hwloc: Refactor error handling
Create common "goto" for sending an error response in resource-hwloc.topo
callback.  Add log for failed RPC too.

@chu11 chu11 force-pushed the chu11:issue495-jansson-cleanup branch from 0aaa0ef to 84cc37b Mar 10, 2017

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Mar 10, 2017

I know 57a29a9 is gone from here now, but one thing to be careful of with boolean in Lua is that 0 (zero) is not false. So not not 0 is true. Disturbing I know, but only nil and false are false.

Let me peek at the flux-cron script and see if we can get it to properly return a boolean for the --kill option.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 10, 2017

@grondo Huh, not what I would have guessed :-)

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 11, 2017

Coverage Status

Coverage increased (+0.006%) to 76.455% when pulling 84cc37b on chu11:issue495-jansson-cleanup into aa5d395 on flux-framework:master.

@garlick garlick merged commit 44fdb7a into flux-framework:master Mar 11, 2017

3 of 4 checks passed

codecov/patch 46.66% of diff hit (target 76.17%)
Details
codecov/project 76.18% (+<.01%) compared to aa5d395
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.006%) to 76.455%
Details
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Mar 11, 2017

I know 57a29a9 is gone from here now, but one thing to be careful of with boolean in Lua is that 0 (zero) is not false. So not not 0 is true. Disturbing I know, but only nil and false are false.

Let me peek at the flux-cron script and see if we can get it to properly return a boolean for the --kill option.

Just an update for @chu11, I think your toboolean solution will work fine in this case. self.opt.k will be either nil or 1, so not not self.opt.k will return expected boolean.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 11, 2017

@grondo Ahh, cool. I'll add a comment to the patch so that's clear. Thanks!

@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.