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

libjsc: misc cleanup #1395

Merged
merged 3 commits into from Mar 28, 2018

Conversation

Projects
None yet
5 participants
@garlick
Copy link
Member

garlick commented Mar 28, 2018

Here are a few simple cleanups for libjsc, peeled off of #1388

The main thing here is dropping the deprecated (and unused) API functions. The rest is minor.

garlick added some commits Mar 22, 2018

libjsc/jstatctl: [cleanup] fix outdated comments
kvs_watch is no longer used internally so drop comment
that refers to it.

The returned "jcb" parameter is now a string, so tell
the user to release it with free(), not json_object_put().
libjsc/jstatctl: [cleanup] drop is_ATTR macros
Private macros to strcmp a key to a jsc attribute
are probably not adding much to readability.
In an effort to make the code more readable, drop these.
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Mar 28, 2018

Nice cleanup! Just to check, I assume this is already tested with flux-sched?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 28, 2018

Coverage Status

Coverage decreased (-0.003%) to 78.802% when pulling 9428a88 on garlick:libjsc_cleanup into bccf233 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 28, 2018

Codecov Report

Merging #1395 into master will decrease coverage by <.01%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master    #1395      +/-   ##
==========================================
- Coverage   78.49%   78.49%   -0.01%     
==========================================
  Files         162      162              
  Lines       29801    29768      -33     
==========================================
- Hits        23392    23365      -27     
+ Misses       6409     6403       -6
Impacted Files Coverage Δ
src/common/libjsc/jstatctl.c 74.86% <86.66%> (-0.18%) ⬇️
src/common/libflux/mrpc.c 85.49% <0%> (-1.18%) ⬇️
src/common/libutil/base64.c 95.07% <0%> (-0.71%) ⬇️
src/broker/module.c 83.79% <0%> (-0.28%) ⬇️
src/common/libflux/message.c 81.48% <0%> (ø) ⬆️
src/broker/overlay.c 74.45% <0%> (+0.31%) ⬆️
src/broker/content-cache.c 74.08% <0%> (+0.64%) ⬆️

@grondo grondo merged commit f7bde37 into flux-framework:master Mar 28, 2018

4 checks passed

codecov/patch 86.66% of diff hit (target 78.49%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +8.17% compared to bccf233
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.003%) to 78.802%
Details
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Mar 28, 2018

Um, I haven't tested it with sched. I assumed that passing the jsc sharness test was probably good enough for this change set. I did check (with grep) that sched wasn't using the deprecated interfaces.

I have been unable to get sched compiled on ubuntu 17.10 (what I currently have in a vm on my laptop) due to the default libboost version of 1.62. I need to figure out how to downgrade that package I guess...

@garlick garlick deleted the garlick:libjsc_cleanup branch Mar 28, 2018

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Mar 28, 2018

I have been unable to get sched compiled on ubuntu 17.10 (what I currently have in a vm on my laptop) due to the default libboost version of 1.62. I need to figure out how to downgrade that package I guess...

Yeah... The defects in some of these libboost versions are pretty unfortunate. I didn't have time to look at ways to work around it. For now, though, because JSC won't affect resource-query, you can just comment out the resource in your make file and test your changes with the sched PR.

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.