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: clean up lookup and txn API's #1253

Merged
merged 17 commits into from Oct 25, 2017

Conversation

Projects
None yet
5 participants
@garlick
Copy link
Member

garlick commented Oct 24, 2017

This PR makes some minor changes to public KVS API functions, and restructures the lookup and txn implementations a bit to be more efficient and maintainable. The public-facing API changes are, first, to add three new functions:

int flux_kvs_txn_put_treeobj (flux_kvs_txn_t *txn, int flags,
                              const char *key, const char *treeobj);
int flux_kvs_lookup_get_symlink (flux_future_t *f, const char **target);
int flux_kvs_lookup_get_treeobj (flux_future_t *f, const char **treeobj);

rather than overloading flux_kvs_txn_put() and flux_kvs_lookup_get(); and second, to relax flux_kvs_txn_put() and flux_kvs_lookup_get() with respect to requiring values to be in encoded JSON form. You can now use those interfaces to put/get any NULL terminated string in the KVS.

The man pages flux_kvs_txn_create(3) and flux_kvs_lookup(3) were revised to reflect these changes, and also cleaned up a bit.

The internal changes are some restructuring to avoid gratuitous JSON encoding/decoding especially when handling flux_kvs_dir_t objects, and teasing apart the lookup_get functions so each is independent of the others. They all build up internal state in a consistent and idempotent way so they can be mixed and matched and called multiple times.

I'll follow this up with a PR to allow flux-kvs to better handle non-JSON values in the KVS.

@garlick garlick force-pushed the garlick:kvs_api_cleanup branch from 5ac394d to 68f7c09 Oct 24, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 24, 2017

Coverage Status

Coverage increased (+0.002%) to 78.523% when pulling 68f7c09 on garlick:kvs_api_cleanup into 4e15246 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 24, 2017

Codecov Report

Merging #1253 into master will increase coverage by 0.02%.
The diff coverage is 83.11%.

@@            Coverage Diff             @@
##           master    #1253      +/-   ##
==========================================
+ Coverage   77.85%   77.88%   +0.02%     
==========================================
  Files         154      154              
  Lines       28870    28934      +64     
==========================================
+ Hits        22478    22534      +56     
- Misses       6392     6400       +8
Impacted Files Coverage Δ
src/common/libkvs/kvs_watch.c 87.33% <100%> (+0.11%) ⬆️
src/common/libjsc/jstatctl.c 78.33% <100%> (ø) ⬆️
src/bindings/lua/flux-lua.c 81.16% <100%> (+0.08%) ⬆️
src/common/libkvs/kvs_dir.c 98.07% <100%> (+0.05%) ⬆️
src/common/libkvs/kvs_lookup.c 75.69% <77.19%> (+0.5%) ⬆️
src/common/libkvs/kvs_txn.c 74.01% <78.37%> (-2.1%) ⬇️
src/common/libkvs/kvs_classic.c 89.5% <80%> (-1.1%) ⬇️
src/cmd/flux-kvs.c 79.77% <83.33%> (+0.03%) ⬆️
src/cmd/builtin/env.c 94.11% <0%> (-5.89%) ⬇️
src/common/libutil/base64.c 95.07% <0%> (-1.71%) ⬇️
... and 9 more
@@ -309,7 +309,7 @@ static int jobid_exist (flux_t *h, int64_t j)
if (path == NULL)
goto done;
if (!(f = flux_kvs_lookup (h, FLUX_KVS_READDIR, path))
|| flux_kvs_lookup_get (f, NULL) < 0) {
|| flux_future_get (f, NULL) < 0) {

This comment has been minimized.

@chu11

chu11 Oct 25, 2017

Contributor

this seems like a different cleanup than what this commit is?

This comment has been minimized.

@garlick

garlick Oct 25, 2017

Author Member

I'm actually not sure why I did that now. Will back it out.

This comment has been minimized.

@garlick

garlick Oct 25, 2017

Author Member

Oops that change was necessary to avoid breaking git bisect, because in this commit flux_kvs_lookup_get() loses the ability to fetch a directory object.

The straightforward change would have been to use flux_kvs_lookup_get_dir() and set the 'dir' parameter to NULL. I'll switch to that.

Testing the future result is just seeing if the RPC returned an error nor not without parsing the directory object, since we only wanted to see if the lookup worked or not here. Needless optimization.

@@ -53,7 +53,6 @@
*
* NULL or empty values:
* A zero length raw value is considered valid.
* A NULL JSON string passed to flux_kvs_txn_put() is interpreted as an unlink.

This comment has been minimized.

@chu11

chu11 Oct 25, 2017

Contributor

Perhaps a comment would be useful stating that NULL is invalid, but if user wishes to put a zero length value, to use flux_kvs_txn_put_raw().

This comment has been minimized.

@garlick

garlick Oct 25, 2017

Author Member

Yes or actually it should be valid to put a NULL there.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 25, 2017

restarted a couple builders. Both seemed to hang during the valgrind test (??)
(Then I realized you have to rebase anyway)

@@ -17,49 +17,71 @@ SYNOPSIS
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_get (flux_future_t *f, const char **result);

This comment has been minimized.

@chu11

chu11 Oct 25, 2017

Contributor

should be char **value if we're going off the header file? Or intentionally going with result?

`flux_future_destroy()` is called.
`flux_kvs_lookup_get()` interprets the result as a NULL-terminated string
value. The value is assigned it to _result_.

This comment has been minimized.

@chu11

chu11 Oct 25, 2017

Contributor

should be adjusted if above change is done.

This comment has been minimized.

@garlick

garlick Oct 25, 2017

Author Member

value is better there IMHO

they be returned without conversion. That is, a "valref" will not
be converted to a "val" object, and a "dirref" will not be converted
to a "dir" object. This is useful for obtaining a snapshot reference
that can be passed to `flux_kvs_lookup_getat()`.

This comment has been minimized.

@chu11

chu11 Oct 25, 2017

Contributor

should be flux_kvs_lookupat()?

This comment has been minimized.

@garlick

garlick Oct 25, 2017

Author Member

yep, thanks for catching that! How did I ever survive without these reviews.

@garlick garlick force-pushed the garlick:kvs_api_cleanup branch from 68f7c09 to 369d65d Oct 25, 2017

`flux_kvs_txn_mkdir()` sets _key_ to an empty directory.
`flux_kvs_txn_unlink()` removes _key_. If _key_ is a directory,
all its contents are removed as well.
`flux_kvs_txn_symlink()` sets _key_ to a symbolic link pointing to _target_,
`flux_kvs_txn_symlink()` sets _key_ to a symbolic link pointing to_target_,

This comment has been minimized.

@chu11

chu11 Oct 25, 2017

Contributor

errant typo, need a space

This comment has been minimized.

@garlick

garlick Oct 25, 2017

Author Member

yep.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 25, 2017

overall looks good to me, just some nits. lots of good cleanup.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 25, 2017

Coverage Status

Coverage increased (+0.03%) to 78.541% when pulling 369d65d on garlick:kvs_api_cleanup into 39fd3cb on flux-framework:master.

garlick added some commits Oct 20, 2017

libkvs/txn: fix memleak in op construction
Problem: if validate_op() fails when adding operation
to transaction, the operation is leaked.

Ensure json_decref() is called on this error path.
libkvs/lookup: add flux_kvs_lookup_get_symlink()
Create a function specifically for retrieving a link
target looked up with FLUX_KVS_READLINK, instead of
overloading flux_kvs_lookup_get().

Update users in lua bindings, flux-kvs command,
and tests.
libkvs/lookup: [cleanup] reduce decoding/encoding
Problem: flux_kvs_lookup_get_dir() internally calls
flux_kvs_lookup_get() which returns the directory object
encoded as a string, then passes it to flux_kvsdir_create(),
which decodes it back into an object.

Make flux_kvs_lookup_get_dir() independent of flux_kvs_lookup_get()
and use flux_kvsdir_create_fromobj() to avoid the extra string
encoding.
libkvs/dir: [cleanup] add private interface
Add
  kvsdir_get_obj()
  kvsdir_create_fromobj()

Prepare for future cleanup that will avoid extra encode/decode
steps within the KVS API implementation.

These functions are defined in a private header and are not
exported as part of the flux public API.
libkvs/dir: [cleanup] reduce decoding/encoding
Problem: flux_kvsdir_copy() calls flux_kvsdir_create()
which only accepts an encoded directory object.

Change flux_kvsdir_copy to call kvsdir_create_fromobj(),
which accepts the source directory object directly, incrementing
its reference count instead of recreating it.
libkvs/dir: [cleanup] drop flux_kvsdir_tostring
Problem: flux_kvsdir_tostring() has only one internal user.

Change flux_kvs_watch_once_dir() to call kvsdir_get_obj()
and treeobj_encode() instead, then drop flux_kvsdir_tostring()
and its usage in the kvsdir unit test.
libkvs/lookup: [cleanup] make get_unpack independent
Problem: flux_kvs_lookup_get_unpack() calls flux_kvs_lookup_get()
internally, make the code hard to follow.

Add the small amount of code to allow flux_kvs_lookup_unpack()
to stand alone.

@garlick garlick force-pushed the garlick:kvs_api_cleanup branch from 369d65d to 3355a02 Oct 25, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 25, 2017

I added on some commits to address the comments so far and rebased on current master.

I'll want to squash those once review is complete.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 25, 2017

Coverage Status

Coverage increased (+0.04%) to 78.538% when pulling 3355a02 on garlick:kvs_api_cleanup into 0f0f5eb on flux-framework:master.

@@ -56,7 +56,7 @@ or interpret it in different ways. Results remain valid until
`flux_future_destroy()` is called.
`flux_kvs_lookup_get()` interprets the result as a NULL-terminated string
value. The value is assigned it to _result_.
value. The value is assigned it to _value.

This comment has been minimized.

@chu11

chu11 Oct 25, 2017

Contributor

should be _value_ w/ extra underscore at end?

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 25, 2017

one tiny nit, other than that, I'd say squash away

@garlick garlick force-pushed the garlick:kvs_api_cleanup branch 2 times, most recently from e8210b3 to 032d22f Oct 25, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 25, 2017

Coverage Status

Coverage increased (+0.08%) to 78.58% when pulling 032d22f on garlick:kvs_api_cleanup into 0f0f5eb on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 25, 2017

Coverage Status

Coverage decreased (-0.02%) to 78.477% when pulling 032d22f on garlick:kvs_api_cleanup into 0f0f5eb on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 25, 2017

Squashed. Fixed the typo found by @chu11 and one more.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 25, 2017

Oops I merged one of those later fixes with the wrong commit. Fixing...

libkvs/lookup: add flux_kvs_lookup_get_treeobj
Create a function specifically for retrieving a
tree object looked up with FLUX_KVS_TREEOBJ, instead of
overloading flux_kvs_lookup_get().

Update users and test cases.

garlick added some commits Oct 23, 2017

libkvs/lookup: validate returned treeobj
Problem: the flux_kvs_lookup_get functions perform only
minimal validation on returned RFC 11 tree objects, which
could lead to programming errors.

Factor out decode_treeobj() from the "get" functions and
perform validation there.
libkvs/txn: [cleanup] avoid extra encode/decode
Problem: each time an operation is appended to the transaction,
it is encoded then validated, which requires it to be decoded
first.

Move validation logic to txn_encode_op(), before encoding.

Also rename internal static function flux_kvs_txn_put_treeobj()
to append_op_to_txn().
libkvs/txn: drop special case for txn_put of NULL
If flux_kvs_txn_put() is handed a NULL value, it calls
flux_kvs_txn_unlink().  This is a bit counter-intuitive now
that, for example, a zero-length value is valid.

Drop the special case from flux_kvs_txn_put(), and make
a NULL value an EINVAL error in this commit.

Add the special case into the deprecated functions
flux_kvs_put() and flux_kvsdir_put() so that lua bindings
continue to work.

Update tests.
libkvs/txn: [cleanup] add flux_kvs_txn_put_treeobj
Problem: it is a bit of a kludge to have flux_kvs_txn_put()
accept the FLUX_KVS_TREEOBJ flag and then clear it so it's
not sent in the commit request.

Add flux_kvs_txn_put_treeobj(), a dedicated function to do that.

Update the flux-kvs and tests.
libkvs/txn: txn_put should not require JSON value
Problem: flux_kvs_txn_put() requires its string value
to be valid JSON, but KVS values are no longer required to
be JSON.

Rename parameter to 'value' and allow it to be any NULL
terminated string.

Retain the JSON requirement in the "classic" function
flux_kvs_put() and flux_kvsdir_put().

Retain the "flux-kvs put" behavior of encoding values as
a JSON string if they aren't already valid encoded JSON.
doc/flux_kvs_lookup(3): add get_symlink, get_treeobj
Add entries for the new functions, flux_kvs_lookup_get_symlink()
and flux_kvs_lookup_get_treeobj().

Split function prototypes over two lines where they didn't fit
comfortably within 80 columns in the rendered text.

Rewrite and refactor descriptions to be more clear.
doc/flux_kvs_txn_create(3): add put_treeobj()
Add entry for flux_kvs_txn_put_treeobj().

Note that flags must always be zero now.

Rewrite and refactor descriptions to be more clear.
libkvs/txn: txn_put accepts a NULL value
Modify flux_kvs_txn_put() to allow a NULL (zero length)
value to be committed.

Update comment about NULL handling.

Update txn unit test to verify that txn_put properly encodes
a NULL value.
libkvs/lookup: lookup_get can return NULL value
A flux_kvs_lookup_get() on a NULL (zero length) value
should set value to NULL and return success.

@garlick garlick force-pushed the garlick:kvs_api_cleanup branch from 032d22f to 29e8f6e Oct 25, 2017

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 25, 2017

Two travis builds were stuck before they even started. I canceled and restarted them.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 25, 2017

Coverage Status

Coverage increased (+0.009%) to 78.506% when pulling 29e8f6e on garlick:kvs_api_cleanup into 0f0f5eb on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 25, 2017

lgtm, ready to merge?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 25, 2017

yep!

@chu11 chu11 merged commit 2694116 into flux-framework:master Oct 25, 2017

4 checks passed

codecov/patch 83.11% of diff hit (target 77.85%)
Details
codecov/project 77.88% (+0.02%) compared to 0f0f5eb
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.009%) to 78.506%
Details
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 25, 2017

Thanks!

@garlick garlick deleted the garlick:kvs_api_cleanup branch Nov 15, 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.