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

libkvs: cleanup, deprecate old functions, update users #1233

Merged
merged 39 commits into from Oct 18, 2017

Conversation

Projects
None yet
5 participants
@garlick
Copy link
Member

garlick commented Oct 11, 2017

This PR is an attempt at some long overdue KVS API cleanup. Not ready for merge consideration until a PR is submitted to update sched.

All KVS API components are prefixed with flux_ now, and the kvs_*, kvsitr_*, and kvsdir_* prefixes are dropped from flux-core symbol exports.

The older API functions defined in kvs_classic.h are marked with __attribute__ ((deprecated)). I stopped short of moving these functions to libcompat, which would take them out of the exported namespace, due to difficulties with the python bindings. The old "typed" get/put functions that remain were replaced with jansson pack/unpack style equivalents to reduce the deprecated API footprint somewhat.

The kvs_dir.h functions, while still not quite right, were cleaned up a bit, including making some flux_kvsdir_t * parameters const that ought to have been.

Some new functions were introduced:

flux_kvsdir_copy()
flux_kvs_lookup_get_vunpack()
flux_kvs_lookup_get_dir()
flux_kvs_txn_vpack()

Finally, inline (header) documentation was added/updated for kvs_dir.h, kvs_watch.h, and kvs_classic.h which are still not quite stable IMHO. Man pages were added for the new lookup and txn functions.

I did not fully modernize the API usage of wreck, libkz, and the lua bindings since some of that code is going to be replaced and it was pretty involved. There will be a few more deprecated function warnings when building those for now.

The kvs_watch.h API is still the old one and in dire need of redesign, but not dealt with here.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 11, 2017

Coverage Status

Coverage decreased (-0.1%) to 78.569% when pulling 7aefbe5 on garlick:kvs_api_names into 5fe021a on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 11, 2017

Hmm, getting some travis failures and I'm having a hard time spotting the root cause. Best I can tell is things are going wrong with distcheck in the python bindings...

[edited: it's pylint unhappy with a long line]

if [ -x "$( which pylint )" ] ; then  pylint --rcfile=../../../../../../src/bindings/python/.pylintrc ../flux ; fi
[snip]
************* Module flux.kvs
I: 97, 0: Locally disabling too-many-ancestors (R0901) (locally-disabled)
I: 97, 0: Locally disabling too-many-public-methods (R0904) (locally-disabled)
C:308, 0: Line too long (88/80) (line-too-long)
[snip]
make: *** [distcheck] Error 
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 15, 2017

Codecov Report

Merging #1233 into master will decrease coverage by 0.08%.
The diff coverage is 74.71%.

@@            Coverage Diff             @@
##           master    #1233      +/-   ##
==========================================
- Coverage   78.12%   78.03%   -0.09%     
==========================================
  Files         154      154              
  Lines       28812    28936     +124     
==========================================
+ Hits        22508    22581      +73     
- Misses       6304     6355      +51
Impacted Files Coverage Δ
src/common/libkvs/kvs_dir.c 98.01% <100%> (+2.01%) ⬆️
src/common/libkvs/kvs_classic.c 90.6% <100%> (+1.33%) ⬆️
src/common/libkvs/kvs.c 64.63% <100%> (-0.49%) ⬇️
src/common/libkvs/kvs_watch.c 87.22% <100%> (ø) ⬆️
src/modules/wreck/job.c 66.89% <49.42%> (-3.66%) ⬇️
src/modules/resource-hwloc/resource.c 67.46% <55.17%> (-4.06%) ⬇️
src/cmd/builtin/hwloc.c 80.36% <58.33%> (-0.53%) ⬇️
src/modules/aggregator/aggregator.c 79.15% <62.5%> (-0.45%) ⬇️
src/bindings/lua/kvs-lua.c 76.59% <71.79%> (ø) ⬆️
src/modules/wreck/wrexecd.c 76.19% <73.84%> (+0.72%) ⬆️
... and 22 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 15, 2017

Coverage Status

Coverage decreased (-0.1%) to 78.603% when pulling 16d135b on garlick:kvs_api_names into 5fe021a on flux-framework:master.

@garlick garlick force-pushed the garlick:kvs_api_names branch from 16d135b to 80eb26a Oct 16, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 16, 2017

Coverage Status

Coverage decreased (-0.2%) to 78.549% when pulling 80eb26a on garlick:kvs_api_names into 2bddc2e on flux-framework:master.

@garlick garlick force-pushed the garlick:kvs_api_names branch from 80eb26a to 447bcdb Oct 16, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 16, 2017

Coverage Status

Coverage decreased (-0.2%) to 78.539% when pulling 447bcdb on garlick:kvs_api_names into 2bddc2e on flux-framework:master.

@garlick garlick requested review from chu11 and grondo Oct 16, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 16, 2017

Pylint problem solved.

Still todo: add some tests to cover missing bits, and put together a patch to update sched.

return NULL;
}
}
return dir->dirobj_string;

This comment has been minimized.

@chu11

chu11 Oct 16, 2017

Contributor

should the change from using dirobj_string to json_str be in a different commit, b/c this doesn't seem to be mapped to this commit message. It appears this also fixes a mem-leak.

This comment has been minimized.

@garlick

garlick Oct 16, 2017

Author Member

Yes that looks like a rebase error. I'll fix...

};

static const char *auxkey = "flux::lookup_ctx";

static void free_ctx (struct lookup_ctx *ctx)
{
if (ctx) {
free (ctx->key);
free (ctx->treeobj_str);

This comment has been minimized.

@chu11

chu11 Oct 16, 2017

Contributor

should we free atref too? Can't see where else it is freed.

This comment has been minimized.

@garlick

garlick Oct 16, 2017

Author Member

Yes good catch! I'll fix that.

@@ -254,8 +254,11 @@ static int l_flux_new (lua_State *L)
static int l_flux_kvsdir_new (lua_State *L)

This comment has been minimized.

@chu11

chu11 Oct 16, 2017

Contributor

commit seems to do more than just update to use flux_kvs_lookup_get_dir. Perhaps commit message should just be enhanced.

This comment has been minimized.

@chu11

chu11 Oct 16, 2017

Contributor

oh yeah, this specific line is not relevant, just didn't know how to comment on a commit message.

This comment has been minimized.

@garlick

garlick Oct 16, 2017

Author Member

Yes that was pretty vague. Changed to

bindings/lua: use flux_kvs_lookup()

Replace use of soon to be deprecated flux_kvs_get()
and flux_kvs_get_dir() with modern flux_kvs_lookup()
equivalents.

@garlick garlick force-pushed the garlick:kvs_api_names branch from 447bcdb to 892b1e2 Oct 17, 2017

*/
int flux_kvs_watch_once (flux_t *h, const char *key, char **json_str);

/* Same as above except value is a direcory pointed to by 'dirp'.

This comment has been minimized.

@chu11

chu11 Oct 17, 2017

Contributor

typo directory

This comment has been minimized.

@chu11

chu11 Oct 17, 2017

Contributor

raced on comments/old commit and github is showing my comment as outdated. typo directory

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 17, 2017

just finished skimming over everything. Overall looks good. Lots of lines of code, so hopefully I didn't miss anything. I think @grondo should look over the wreck and job areas that were updated, as I don't know that code as well.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 17, 2017

Coverage Status

Coverage decreased (-0.1%) to 78.54% when pulling 892b1e2 on garlick:kvs_api_names into 9f03dda on flux-framework:master.

@garlick garlick force-pushed the garlick:kvs_api_names branch from 892b1e2 to f2cdd98 Oct 17, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 17, 2017

Thanks - fixed the "directory" typo, squashed and pushed.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 17, 2017

Coverage Status

Coverage decreased (-0.1%) to 78.575% when pulling f2cdd98 on garlick:kvs_api_names into 9f03dda on flux-framework:master.

@grondo

grondo approved these changes Oct 17, 2017

Copy link
Contributor

grondo left a comment

Looks fine to me. Just a couple comments on flux_kvs_lookup_get_dir -- not sure if they were appropriate for this PR, but just wanted to feel like I was contributing ;-)

@@ -53,6 +55,9 @@ the returned JSON is parsed according to variable arguments in Jansson
`flux_kvs_lookup_get_raw()` is identical to `flux_kvs_lookup_get()` except
the raw value is returned without decoding.
`flux_kvs_lookup_get_dir()` is identical to `flux_kvs_lookup_get()` except
a directory object is returned.

This comment has been minimized.

@grondo

grondo Oct 17, 2017

Contributor

I know you are considering removal of the kvsdir abstraction, but in the context of the addition of flux_kvs_lookup_get_dir it might be useful to consider addition of a refcount to kvsdir objects. This would avoid the necessary copies in the common case where we want the kvsdir to persist, but we're done with the future.

This comment has been minimized.

@garlick

garlick Oct 17, 2017

Author Member

We do have flux_kvsdir_incref() but since the object returned from the lookup is const (indicating that it still belongs to the future), it seems a bit wrong to allow its reference count to be modified? This is why I added flux_kvsdir_copy(), but I intended that to be short lived until the various places where flux_kvsdir_t was used for purposes other than "list of names in directory" were converted to something else TBD.

As we just discussed offline, possibly we need to come up with a new object that serves as a handle to the KVS, and contains things like a "current working directory", a snapshot reference, or a namespace handle, as well as the broker handle, and then all KVS functions would take one of those instead of the broker handle... I'll open an issue on that idea.

|| !(f = flux_kvs_lookup (h, FLUX_KVS_READDIR, key))
|| flux_kvs_lookup_get_dir (f, &dir) < 0
|| !(t->kvs = flux_kvsdir_copy (dir))) {
if (errno != ENOENT)

This comment has been minimized.

@grondo

grondo Oct 17, 2017

Contributor

this chunk of code is repeated twice at least in this source file, maybe it could be its own static function? In general, the lookup->get_dir->copy_dir seems to be common enough idiom that I'm wondering why there isn't a function in the API for it, but it isn't quite common enough for me to care (especially if the flux_kvsdir_t is going to go away.)

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 17, 2017

One other thing. Looks like flux_kvs_pack isn't covered in testing, and that it is only necessary due to untested wrexecd case where no-aggregate-task-exit option is used.

Maybe we can just drop that part of wrexecd and remove flux_kvs_pack? If there is another perceived need for flux_kvs_pack, then should it have a unit test?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 17, 2017

One other thing. Looks like flux_kvs_pack isn't covered in testing, and that it is only necessary due to untested wrexecd case where no-aggregate-task-exit option is used.

Maybe we can just drop that part of wrexecd and remove flux_kvs_pack? If there is another perceived need for flux_kvs_pack, then should it have a unit test?

I'm good with that - let me see if I can do that really quick. o/w I will add a test.

@garlick garlick force-pushed the garlick:kvs_api_names branch from f2cdd98 to 06cd847 Oct 17, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 17, 2017

Dropped flux wreckrun --no-aggregate-task-exit as discussed, and eliminated flux_kvs_pack() from the set of deprecated functions. Squashed that down and re-pushed.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 17, 2017

Coverage Status

Coverage decreased (-0.02%) to 78.66% when pulling 06cd847 on garlick:kvs_api_names into 9f03dda on flux-framework:master.

@garlick garlick force-pushed the garlick:kvs_api_names branch from 06cd847 to 8101399 Oct 18, 2017

libkvs/dir: drop _struct suffix per RFC 7
Problem: "struct kvsdir_struct" and "struct kvsitr_struct"
do not comply with RFC 7.

Drop the "_struct" suffix.

garlick added some commits Oct 5, 2017

libkz: modernize KVS API usage
Modernize some aspects of libkz's KVS API usage.

I held back converting the "anonymous" transactions
to explicit ones, as doing so seemed to hang up the
wreck tests.  My best guess is that something in wrexecd
that is written to the anonymous transaction is actually
being committed by an anonymous commit/fence in libkz.
Since this code is due to be replaced it didn't seem like
time well spent right now to run this down.
libkvs/classic: add flux_kvsdir_pack()
Add a function that can replace the individual typed
flux_kvsdir_put_*() functions, thus reducing the
number of functions to be deprecated a little bit.
libkvs/classic: drop flux_kvs_put_<type> functions
Drop
  flux_kvs_put_string()
  flux_kvs_put_int()
  flux_kvs_put_int64()

These functions have no users
libkvs/classic: drop flux_kvsdir_put_<type> funcs
Drop
  flux_kvsdir_put_string()
  flux_kvsdir_put_int()
  flux_kvsdir_put_int64()
  flux_kvsdir_put_double()
  flux_kvsdir_put_boolean()

Switch wrexecd, dtree, and lua bindings over to
flux_kvsdir_pack().
libkvs/lookup: add flux_kvs_lookup_get_dir()
Problem: it takes three functions to fetch a directory:
flux_kvs_lookup(), flux_kvs_lookup_get(), and flux_kvsdir_create().
The caller must then free a future and the flux_kvsdir_t.

Add flux_kvs_lookup_get_dir() that combines the last two steps
and makes the flux_kvsdir_t owned by the future.
libkvs/dir: add flux_kvsdir_copy()
Problem: flux_kvs_get_dir() returns a flux_kvsdir_t that the
caller must free, while flux_kvs_lookup_get_dir() will return a
const one that remains the property of the future. Migrating
from the latter to the former is complicated by the lack of
a flux_kvsdir_copy() function.

Add flux_kvsdir_copy() and a unit test.
libkvs/classic: add missing const
Problem: several "classic" KVS API functions accept
flux_kvsdir_t parameters which they do not modify.

Tag these parameters const to make that clearer.
libkvs/classic: use lookup_get_dir internally
Reimplement flux_kvs_get_dir() and flux_kvsdir_get_dir()
internally in terms of flux_kvs_lookup_get_dir().
libkvs/classic: add some inline documentation
Problem: there is no current documentation for classic KVS
functions, yet they have not been deprecated.

Add some inline documentation to the header, with pointers
to the newer functions that replace them.
libkvs/dir: add some inline documentation
Problem: There is no documentation for flux_kvsdir_t.

The flux_kvsdir_t probably won't be deprecated although it may
be slimmed down in the future.  Meanwhile there are still users.
Start with some inline documentation in the header.
libkvs/watch: add inline documentation
Problem: there is no current documentation for
flux_kvs_watch functions.

These functions are liable to change, so it is probably not
a good use of time to create a manual page at this point.
Update the inline header documentation as a stop gap measure.
bindings/lua: use flux_kvs_lookup()
Replace use of soon to be deprecated flux_kvs_get()
and flux_kvs_get_dir() with modern flux_kvs_lookup()
equivalents.
libkvs/classic: deprecate all functions
Add __attribute__ ((deprecated)) to all kvs_classic functions.
libkvs/dir: skip re-encode in flux_kvsdir_tostring()
flux_kvsdir_tostring() calls json_dumps () on the encoded
JSON object stored inside the object.  Instead of doing this,
just save the original json_str passed to flux_kvsdir_create().

This also fixes a memory leak since the cached json_dumps()
result was not freed when the object was freed.
libkvs/dir: add missing const
Problem: some functions in kvs_dir.h accept a flux_kvsdir_t
parameter that is not const, but do not modify it.

Add const to clarify the interface.
libkvs/lookup: add missing free()
Problem: flux_kvs_lookupat() leaks ctx->atref on destroy.

Add the missing free().
modules/wreck/wrexecd: drop no-aggregate-task-exit
Drop wrexecd's code that handles the flux-wreckrun(1)
and flux-submit() --no-aggregate-task-exit option which is
described as

  Do not use aggregator for task exit messages.  This option
  will result in each task's exit status being committed
  separately into the kvs.

This was the only user of flux_kvs_put_int(), and @grondo thought
we could safely get rid of it.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 18, 2017

Coverage Status

Coverage decreased (-0.07%) to 78.635% when pulling 8101399 on garlick:kvs_api_names into e1a1d87 on flux-framework:master.

@grondo grondo merged commit 778e42b into flux-framework:master Oct 18, 2017

2 of 4 checks passed

codecov/patch 74.71% of diff hit (target 78.12%)
Details
codecov/project 78.03% (-0.09%) compared to e1a1d87
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.07%) to 78.635%
Details

@garlick garlick deleted the garlick:kvs_api_names branch Oct 18, 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.