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

kvs: add flux_kvs_lookup_get_raw() #1218

Merged
merged 10 commits into from Sep 29, 2017

Conversation

Projects
None yet
5 participants
@garlick
Copy link
Member

garlick commented Sep 28, 2017

This PR adds flux_kvs_lookup_get_raw() to the KVS API, to go with flux_kvs_txn_put_raw(), then modifies t/kvs/basic to use them instead of an extra base64 encoding layer for copy-tokvs and copy-fromkvs subcommands. Finally add a couple of sharness tests to fill test gaps identified in #1214 (lookup unencoded content) and #1215 (zero length value).

This is based on top of #1214

@garlick garlick force-pushed the garlick:raw_api branch from 4901d4d to 0be8dff Sep 28, 2017

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Sep 28, 2017

LGTM, will hit button after it passes.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Sep 28, 2017

actually, one nit, I have a # TODO - convert to using "flux content store", see issue1216 comment in t1002-kvs-extra.t. How about axing that test and replacing it with the one you just wrote.

@garlick garlick force-pushed the garlick:raw_api branch from 0be8dff to 2d85019 Sep 28, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 28, 2017

Ah sorry I missed that. That commit is now just a one line change to the test (and removal of the TODO).

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 28, 2017

Hang on typo in that one, fix coming.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 28, 2017

Coverage Status

Coverage increased (+0.01%) to 78.639% when pulling 0be8dff on garlick:raw_api into 19d06f1 on flux-framework:master.

@garlick garlick force-pushed the garlick:raw_api branch from 2d85019 to 464af50 Sep 28, 2017

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 28, 2017

Does the new API function need an entry in flux_kvs_lookup.adoc?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 28, 2017

Yes :-/

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 28, 2017

Coverage Status

Coverage increased (+0.008%) to 78.635% when pulling 464af50 on garlick:raw_api into 19d06f1 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 28, 2017

Codecov Report

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

@@            Coverage Diff             @@
##           master    #1218      +/-   ##
==========================================
- Coverage   78.21%   78.21%   -0.01%     
==========================================
  Files         158      158              
  Lines       29283    29297      +14     
==========================================
+ Hits        22904    22914      +10     
- Misses       6379     6383       +4
Impacted Files Coverage Δ
src/common/libkvs/kvs_txn.c 78.14% <ø> (ø) ⬆️
src/common/libkvs/kvs_lookup.c 74.1% <86.66%> (+1.94%) ⬆️
src/common/libutil/blobref.c 97.22% <0%> (-1.39%) ⬇️
src/common/libflux/request.c 87.17% <0%> (-1.29%) ⬇️
src/common/libflux/rpc.c 93.38% <0%> (-0.83%) ⬇️
src/broker/content-cache.c 73.43% <0%> (-0.65%) ⬇️
src/common/libflux/message.c 81.25% <0%> (-0.36%) ⬇️
src/common/libcompat/rpc.c 94.57% <0%> (-0.05%) ⬇️
src/common/libflux/future.c 88.31% <0%> (+0.46%) ⬆️
src/common/libutil/base64.c 95.77% <0%> (+0.7%) ⬆️
... and 4 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 28, 2017

Coverage Status

Coverage increased (+0.0007%) to 78.628% when pulling 464af50 on garlick:raw_api into 19d06f1 on flux-framework:master.

@garlick garlick force-pushed the garlick:raw_api branch from 464af50 to 86d091a Sep 29, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 29, 2017

OK, man pages all straightened out for these functions, plus fixed some Makefile.am errors in the man3 directory (broken out to separate commits)

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 29, 2017

Coverage Status

Coverage increased (+0.01%) to 78.639% when pulling 86d091a on garlick:raw_api into 19d06f1 on flux-framework:master.

@@ -31,6 +31,10 @@ SYNOPSIS
int flux_kvs_txn_symlink (flux_kvs_txn_t *txn, int flags,
const char *key, const char *target);

int flux_kvs_txn_put_raw (flux_kvs_txn_t *txn, int flags,
const char *key, void *data, int len);

This comment has been minimized.

@chu11

chu11 Sep 29, 2017

Contributor

const void *data?

This comment has been minimized.

@garlick

garlick Sep 29, 2017

Author Member

I think you might be looking at an older version there? This is already done.

I probably shouldn't have forced pushed a squashed version back there.

This comment has been minimized.

@chu11

chu11 Sep 29, 2017

Contributor

still shows up void * to me. This is the adoc, not the header file.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Sep 29, 2017

also flux_kvs_lookup_get_raw() can be added to libkvs/test/kvs_lookup.c to capture the bad input case.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 29, 2017

Coverage Status

Coverage increased (+0.07%) to 78.699% when pulling f989abb on garlick:raw_api into 19d06f1 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 29, 2017

Ah thanks, just pushed a fix.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 29, 2017

Coverage Status

Coverage decreased (-0.006%) to 78.621% when pulling 6b7e8b6 on garlick:raw_api into 19d06f1 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 29, 2017

Restarted one builder due to #1145 write error.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 29, 2017

@chu11 and @grondo, if you're OK with this I'll squash it.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Sep 29, 2017

looks good to me, squash away

garlick added some commits Sep 29, 2017

libkvs/test: cover flux_kvs_lookup_get_raw() EINVAL
Call flux_kvs_lookup_get_raw() with bad parameters and make
sure it fails with errno == EINVAL.
t/kvs/basic: copy-* subcommand use raw API
Problem: copy-tokvs and copy-fromkvs use custom JSON
encapsulated base64 encoding to store raw data.

Use flux_kvs_txn_put_raw() and flux_kvs_lookup_get_raw().
t/kvs: add test for raw content
Add test that creates a 'valref' entry in the KVS
pointing to content stored out of band.  Read the raw
content back in with flux_kvs_lookup_get_raw().

Fixes #1216
t/kvs: test zero size raw values
Store a zero length raw value and read it back.

Fixes #1215

garlick added some commits Sep 28, 2017

doc/flux_kvs_lookup(3): fix man aliases
Problem: Makefile.am dependencies were not updated when
lookup functions were renamed.

Fix names.
doc/flux_kvs_txn_create(3): fix man aliases
Problem: Makefile.am dependencies were not added for
flux_kvs_txn_symlink(3).

Add the dependency.

@garlick garlick force-pushed the garlick:raw_api branch from 6b7e8b6 to 5326555 Sep 29, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 29, 2017

Thanks, squashed.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Sep 29, 2017

LGTM, will hit button after passes travis

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 29, 2017

Coverage Status

Coverage decreased (-0.003%) to 78.624% when pulling 5326555 on garlick:raw_api into 19d06f1 on flux-framework:master.

@chu11 chu11 merged commit 96a165c into flux-framework:master Sep 29, 2017

4 checks passed

codecov/patch 86.66% of diff hit (target 78.21%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +8.45% compared to 19d06f1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.003%) to 78.624%
Details

@garlick garlick deleted the garlick:raw_api branch Oct 2, 2017

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

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.