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

clean up KVS API implementation #1099

Merged
merged 19 commits into from Jul 4, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Jun 30, 2017

This PR divides up the KVS API into separate source files, reimplements some parts in terms of jansson, addresses some exit-on-malloc-failure behavior, and takes a small step towards the API changes proposed in #1094, implementing these lookup functions:

flux_future_t *flux_kvs_lookup (flux_t *h, int flags, const char *key);
flux_future_t *flux_kvs_lookupat (flux_t *h, int flags, const char *key,
                                  const char *treeobj);

int flux_kvs_lookup_get (flux_future_t *f, const char **json_str);
int flux_kvs_lookup_getf (flux_future_t *f, const char *fmt, ...);

Apart from those new functions, this is mostly cleanup. #1094 goes much further in its proposed changes, but I don't think that design has settled yet, and this PR is big enough it should probably stand alone.

Still todo: man pages for the above functions.

garlick added some commits Jun 28, 2017

libkvs/deprecated: reimplement deprecated functions
Reimplement deprecated json-c based API functions in
a more standalone way, in terms of newer public interfaces.
libkvs/classic: reimplement classic get functions
Reimplement kvs_get() and type-specific convenience
functions in terms of new lookup API.  We will phase
these into deprecated status later on.
modules/kvs: validate "get" input
Problem:  the client API implementation of "getat",
validates that the root treeobj, but the server accepts
anything.  This leaves the KVS module somewhat vulnerable
malformed input.

Move validation to the KVS module.
libkvs/dir: reimplement kvsdir_t operations
Unwind kvsdir_t details from kvs.c.  Make it an opaque
type to the rest of the KVS client API implementation.
Reimplement using jansson.
test/t1002-kvs-cmd.t: cleanup
This test runs --verbose, these errors are omitted:

./t1002-kvs-cmd.t: 352: [: 0: unexpected operator
./t1002-kvs-cmd.t: 352: [: 0: unexpected operator
./t1002-kvs-cmd.t: 380: [: 1: unexpected operator

Use [ $i -eq 50 ] to compare numbers rather
than [ $i == "50" ].  I'm not sure what, if any, effect
this actually had on the tests.  I seemed to have some
intermediate failures that went away after fixing this,
so possibly the while loop was exiting on the first
iteration, and that worked most of the time.

Also, tidy up here documents by replacing << with <<-
and indending them.
libkvs: simplify kvs_copy()
Now that all the other "get" functions have been reimplemented,
the last user of the internal function getobj() is ks kvs_copy().
Implement it using the new lookup functions and get rid of getobj().
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 30, 2017

Codecov Report

Merging #1099 into master will increase coverage by 0.26%.
The diff coverage is 84.15%.

@@            Coverage Diff            @@
##           master   #1099      +/-   ##
=========================================
+ Coverage   78.04%   78.3%   +0.26%     
=========================================
  Files         151     156       +5     
  Lines       26115   26084      -31     
=========================================
+ Hits        20381   20426      +45     
+ Misses       5734    5658      -76
Impacted Files Coverage Δ
src/modules/kvs/kvs.c 79.81% <100%> (+0.02%) ⬆️
src/common/libkvs/kvs_deprecated.c 38.18% <38.18%> (ø)
src/common/libkvs/kvs_dir.c 79.45% <79.45%> (ø)
src/common/libkvs/kvs_lookup.c 80% <80%> (ø)
src/common/libkvs/kvs.c 80.06% <84.31%> (+4.57%) ⬆️
src/common/libkvs/kvs_classic.c 89.14% <89.14%> (ø)
src/common/libkvs/kvs_watch.c 93.46% <93.46%> (ø)
src/common/libflux/mrpc.c 85.31% <0%> (-1.2%) ⬇️
src/common/libflux/handle.c 85.49% <0%> (-0.52%) ⬇️
src/broker/module.c 83.66% <0%> (-0.29%) ⬇️
... and 8 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 30, 2017

Coverage Status

Changes Unknown when pulling 45022c6 on garlick:kvsapi_cleanup into ** on flux-framework:master**.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jun 30, 2017

It appears from coverage that some of the kvs_watch functions aren't covered. For the deprecated functions, I wonder if this would be a good point to excise unused code (e.g. kvs_watch_once_obj and its helpers).

Other untested interfaces appear to include kvs_get_boolean and kvsdir_get_boolean, kvs_watch_{string,int64,double,boolean} and kvs_watch_once_int

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jun 30, 2017

Good idea excising unused code. Maybe rather than writing tests for the uncovered interfaces that will likely go away, I can switch users over to covered interfaces...

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jun 30, 2017

Good point. Let me know if you need any help with that.

garlick added some commits Jul 1, 2017

libkvs/classic: drop kvs[dir]_get_boolean
Drop these functions that have no users:

kvs_get_boolean()
kvsdir_get_boolean()

Update "getas" test driver to drop boolean support
libkvs/watch: drop kvs_watch_int
Move test users over to kvs_watch, then drop kvs_watch_int
which has no non-test users.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 2, 2017

Coverage Status

Changes Unknown when pulling 879962a on garlick:kvsapi_cleanup into ** on flux-framework:master**.

@garlick garlick force-pushed the garlick:kvsapi_cleanup branch from b787b5b to 2a1c194 Jul 3, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 3, 2017

Coverage Status

Changes Unknown when pulling 3718a7a on garlick:kvsapi_cleanup into ** on flux-framework:master**.

@grondo grondo merged commit f2cf6be into flux-framework:master Jul 4, 2017

4 checks passed

codecov/patch 84.15% of diff hit (target 78.04%)
Details
codecov/project 78.3% (+0.26%) compared to 6c555ee
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 78.562%
Details

@garlick garlick deleted the garlick:kvsapi_cleanup branch Jul 4, 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.