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

Extend JSC to add R_lite support #1485

Merged
merged 5 commits into from
Apr 25, 2018
Merged

Conversation

dongahn
Copy link
Member

@dongahn dongahn commented Apr 24, 2018

This PR supports the R_lite writer PR which I will post soon to flux-sched.

Also add some minor fixes along the way:

Add a very simple R_lite test to a JSC test.

@coveralls
Copy link

coveralls commented Apr 24, 2018

Coverage Status

Coverage decreased (-0.06%) to 78.973% when pulling 9a4ba90 on dongahn:R_lite_jsc into 86d5b32 on flux-framework:master.

@codecov-io
Copy link

codecov-io commented Apr 24, 2018

Codecov Report

Merging #1485 into master will decrease coverage by 0.05%.
The diff coverage is 47.36%.

@@            Coverage Diff             @@
##           master    #1485      +/-   ##
==========================================
- Coverage   78.72%   78.66%   -0.06%     
==========================================
  Files         164      164              
  Lines       30358    30405      +47     
==========================================
+ Hits        23900    23919      +19     
- Misses       6458     6486      +28
Impacted Files Coverage Δ
src/common/libjsc/jstatctl.c 72.35% <40.81%> (-1.92%) ⬇️
src/cmd/flux-jstat.c 85.84% <87.5%> (ø) ⬆️
src/common/libflux/response.c 83.73% <0%> (-0.82%) ⬇️
src/modules/kvs/kvs.c 65.87% <0%> (-0.17%) ⬇️
src/bindings/lua/flux-lua.c 81.58% <0%> (-0.09%) ⬇️
src/common/libflux/message.c 81.48% <0%> (+0.23%) ⬆️
src/broker/overlay.c 74.13% <0%> (+0.31%) ⬆️
src/common/libutil/base64.c 95.77% <0%> (+0.7%) ⬆️

@dongahn
Copy link
Member Author

dongahn commented Apr 24, 2018

Just posted the corresponding flux-sched PR: flux-framework/flux-sched#321

@garlick
Copy link
Member

garlick commented Apr 24, 2018

@dongahn, this looks like it solves the problem, and I'm glad you added the test.

I think there are quite a few issues that shouldn't blow by without comment, such as

  • logging within a library
  • embedded synchronous RPCs
  • use of json-c
  • use of functions that assert on malloc failure

However, this code is consistent with hte rest of the library and I think we could merge it as is with the plan to create something new for the new execution system. Is that reasonable?

@dongahn
Copy link
Member Author

dongahn commented Apr 25, 2018

That sounds reasonable to me. Thanks @garlick.

@garlick garlick merged commit 01e2403 into flux-framework:master Apr 25, 2018
@grondo grondo mentioned this pull request May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants