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

[cleanup] convert KVS users to new API and jansson, drop some old API functions #1123

Merged
merged 12 commits into from Jul 25, 2017

Conversation

Projects
None yet
6 participants
@garlick
Copy link
Member

garlick commented Jul 24, 2017

This PR converts some KVS users to new API functions and jansson, and removes several "classic" functions: kvs_move(), kvs_copy(), and kvsdir_symlink() from the API.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jul 24, 2017

No users of the removed functions in flux-sched, by the way.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 25, 2017

Coverage Status

Coverage decreased (-0.008%) to 78.436% when pulling d342272 on garlick:kvs_cleanup3 into ccf25dc on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 25, 2017

Codecov Report

Merging #1123 into master will increase coverage by <.01%.
The diff coverage is 75.9%.

@@            Coverage Diff             @@
##           master    #1123      +/-   ##
==========================================
+ Coverage   78.03%   78.04%   +<.01%     
==========================================
  Files         184      184              
  Lines       31336    31430      +94     
==========================================
+ Hits        24453    24528      +75     
- Misses       6883     6902      +19
Impacted Files Coverage Δ
src/common/libkvs/kvs_classic.c 89.26% <ø> (+1.57%) ⬆️
t/kvs/commitmerge.c 75% <62.5%> (+2.18%) ⬆️
t/kvs/basic.c 75.78% <72.65%> (+3.55%) ⬆️
src/cmd/flux-kvs.c 75% <80.53%> (+0.29%) ⬆️
src/cmd/builtin/env.c 94.11% <0%> (-5.89%) ⬇️
t/rpc/util.c 60.27% <0%> (-3.19%) ⬇️
src/common/libkvs/jansson_dirent.c 80.55% <0%> (-2.78%) ⬇️
src/common/libflux/ev_flux.c 97.56% <0%> (-2.44%) ⬇️
src/modules/kvs/waitqueue.c 86.45% <0%> (-2.09%) ⬇️
src/modules/kvs/commit.c 81.81% <0%> (-1.36%) ⬇️
... and 23 more
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 25, 2017

Hmm, t/kvs/* should not be included in coverage report. I'll look into that. Seems like great cleanup to me, but I'll defer to @chu11 for this one.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jul 25, 2017

I'm not super happy iwth my commit comments in this PR. Let me fix those and do a rebase, then maybe this can be considered for a merge.

garlick added some commits Jul 21, 2017

cmd/flux-kvs: convert to jansson
Convert the flux-kvs command to use jansson instead of json-c.

Because jansson encoding is slightly different, some whitespace
in expected results had to be tweaked in t1002-kvs-cmd.t.
cmd/flux-kvs: convert to new KVS API
Where possible, convert the flux-kvs command from "classic"
KVS API calls to the new KVS API calls.
t/kvs/basic: convert to jansson
Convert t/kvs/basic to from json-c to jansson.

Since jansson string encoding is a bit different
than json-c's, some whitespace in expected output
in t1000-kvs-basic.t had to be adjusted.
t/kvs/basic: convert to new KVS API
Convert the test program t/kvs/basic from "classic"
KVS API functions to the new ones where possible.
t/kvs/commit: convert to jansson
Convert the t/kvs/commit test program from json-c to jansson.

JSON was only used to support the -s option, which outputs
test statistics as a pretty-printed JSON object.  The -s option
is not used by any of the sharness drivers so this doesn't have
much of an effect.
t/kvs/commit: convert to new KVS API
Convert the t/kvs/commit test program from "classic" KVS API
functions to the new ones where possible.
t/kvs/commitmerge: convert to new KVS API
Convert the test program t/kvs/commitmerge from "classic"
KVS API functions to the new ones where posible.
t1000-kvs-basic.t: drop racy kvs watch tests
These tests are broken and tricky to get right.

They seem to be redundant with the more sophisticated
sharness tests in t1002-kvs-cmd.t, and with the
t/kvs/watch mt test for verifying that watch callbacks
track >1 change to a key, which is run by
the t1000-kvs-basic.t sharness driver.

Rather than fix the tests, I think they can just go
away without impacting coverage.

Fixes #1120

@garlick garlick force-pushed the garlick:kvs_cleanup3 branch from d342272 to 035cd28 Jul 25, 2017

garlick added some commits Jul 24, 2017

t/kvs/basic: drop unused watch, watch-dir subcmds
Now that t1000-kvs-basic.t is no longer driving the
t/kvs/basic watch and watch-dir subcommands, they
have no users and can be removed.
libkvs/classic: drop kvs_move()
This function has no users.  Drop.
libkvs/classic: drop kvs_copy()
This function has no users.  Drop.
libkvs/classic: drop kvsdir_symlink()
This function has no users.  Drop.
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jul 25, 2017

Hmm, t/kvs/* should not be included in coverage report.

Does this pattern in Makefile.am match t/kvs/*.c?

CODE_COVERAGE_IGNORE_PATTERN = \
        "$(abs_top_builddir)/t/*" \
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 25, 2017

Does this pattern in Makefile.am match t/kvs/*.c?

Sorry should have been more clear, that's only for the lcov based coveralls report. Unfortunately, codecov.io requires its list of ignore patterns directly in codecov.yml. I found the incorrect pattern and fixed it in #1125

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 25, 2017

I'm willing to merge if @chu11 gives an ACK.

@chu11
Copy link
Contributor

chu11 left a comment

just a few nits

case json_type_string:
printf ("%s\n", json_object_get_string (o));
case JSON_INTEGER:
printf ("%lld\n", (long long)json_integer_value (o));

This comment has been minimized.

@chu11

chu11 Jul 25, 2017

Contributor

Awhile back I think we started using the PRI macros. I think PRId64 is the right one here.

This comment has been minimized.

@grondo

grondo Jul 25, 2017

Contributor

I would also accept "%jd" and cast to intmax_t or "%ju" and cast to uintmax_t. I personally prefer this and find the PRI macros horrible for code readability. However, I realize I'm in the minority here.

In this case I don't actually see a problem casting json_integer_value() to (long long), what is exactly wrong with it?

This comment has been minimized.

@dongahn

dongahn Jul 25, 2017

Contributor

Yeah, I personally find @grondo's preference much easier to remember. I always had to go back to a book to remember those PRI macros.

This comment has been minimized.

@chu11

chu11 Jul 25, 2017

Contributor

There's nothing wrong with the cast per se. I just recall in #940 that we seemed to convert a ton of stuff to use the PRI macros. So just wanted to be consistent.

This comment has been minimized.

@garlick

garlick Jul 25, 2017

Author Member

I think #940 did solve some problems in the code. There isn't an actual problem here, so maybe we should just pass over this issue for now? We can always open a bug later on against RFC 7 if people do think the approach to this problem should be consistent.

This comment has been minimized.

@chu11

chu11 Jul 25, 2017

Contributor

@garlick sounds good

case json_type_int:
printf ("%d\n", json_object_get_int (o));
case JSON_INTEGER:
printf ("%lld\n", (long long)json_integer_value (o));

This comment has been minimized.

@chu11

chu11 Jul 25, 2017

Contributor

likewise

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Jul 25, 2017

overall LGTM. I did notice that some of the code in t/kvs/basic is getting really close to that of src/cmd/flux-kvs. At some point (not necessarily this PR), is it worthwhile to just axe some of the code in basic? Do we really need some those tests anymore? If it's cleanup for another day, we can probably just make an issue for it.

@garlick garlick force-pushed the garlick:kvs_cleanup3 branch from d1f5841 to 1dfcd38 Jul 25, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jul 25, 2017

I went ahead and used JSON_INTEGER_FORMAT as the printf format token for json_integer_value() as described in the jansson docs.

PRId64 didn't quite work here. I guess jansson explicitly defines json_int_t as a long long, but since we're on a 64 bit system, PRId64 is defined as %ld (since longs are 64 bit), but since the two don't match, gcc is sad:

kvs/basic.c:251:17: error: format ‘%ld’ expects argument of type ‘long int’, but argument 2 has type ‘json_int_t {aka long long int}’ [-Werror=format=]
         printf ("%" PRId64 "\n", json_integer_value (o));
                 ^
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jul 25, 2017

That's ok JSON_INTEGER_FORMAT is even longer, so it must be better.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jul 25, 2017

Ooops sorry @grondo, I missed your code comment because apparently I forced a push and made it outdated.

I'll just drop those two commits if @chu11 is OK with that. I hate those macros like everybody else I guess.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 25, 2017

Coverage Status

Coverage increased (+0.09%) to 78.532% when pulling 1dfcd38 on garlick:kvs_cleanup3 into ccf25dc on flux-framework:master.

@garlick garlick force-pushed the garlick:kvs_cleanup3 branch from 1dfcd38 to 035cd28 Jul 25, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jul 25, 2017

OK I dropped them and forced a push.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 25, 2017

Coverage Status

Coverage decreased (-0.03%) to 78.41% when pulling 035cd28 on garlick:kvs_cleanup3 into ccf25dc on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jul 25, 2017

One of the travis tests was stuck - supposedly running, but with no start time and no output. I cancelled that one and restarted and it passed.

I think this is ready for merging?

@grondo grondo merged commit a132370 into flux-framework:master Jul 25, 2017

3 of 4 checks passed

codecov/patch 75.9% of diff hit (target 78.03%)
Details
codecov/project 78.04% (+<.01%) compared to ccf25dc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 78.41%
Details

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

@garlick garlick deleted the garlick:kvs_cleanup3 branch Sep 6, 2017

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.