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: flux_kvs_lookupat() should not assume namespaces and always take rootref #1971

Merged
merged 7 commits into from Feb 1, 2019

Conversation

@chu11
Copy link
Contributor

commented Jan 30, 2019

flux_kvs_lookupat() would lookup a namespace, which does not make sense given it takes a rootref. Also, the rootref was optional and would call flux_kvs_lookup() if it was not set. This doesn't really make sense. So require a root reference to be passed in, don't call flux_kvs_lookup(), and don't lookup a namespace.

As a side effect, don't send a namespace in the lookup RPC if an at reference is mentioned. So in the kvs module, make it so a lookup rpc is required to specify a namespace OR a root reference, but does not require both.

Also fix up some stupid documentation and typos found along the way.

@codecov-io

This comment has been minimized.

Copy link

commented Jan 30, 2019

Codecov Report

Merging #1971 into master will increase coverage by 0.04%.
The diff coverage is 76%.

@@            Coverage Diff             @@
##           master    #1971      +/-   ##
==========================================
+ Coverage      80%   80.04%   +0.04%     
==========================================
  Files         195      195              
  Lines       34986    35005      +19     
==========================================
+ Hits        27990    28020      +30     
+ Misses       6996     6985      -11
Impacted Files Coverage Δ
src/modules/wreck/wrexecd.c 76.74% <0%> (ø) ⬆️
src/modules/kvs-watch/kvs-watch.c 79.05% <100%> (ø) ⬆️
src/modules/kvs/kvs.c 66.33% <100%> (+0.11%) ⬆️
src/modules/kvs/lookup.c 80.98% <100%> (+0.14%) ⬆️
src/common/libkvs/kvs_classic.c 87.71% <66.66%> (-0.91%) ⬇️
src/common/libkvs/kvs_lookup.c 82.48% <66.66%> (+0.61%) ⬆️
src/cmd/flux-kvs.c 82.46% <71.42%> (-0.35%) ⬇️
src/common/libflux/message.c 81.15% <0%> (-0.37%) ⬇️
src/common/libzio/zio.c 74% <0%> (-0.23%) ⬇️
src/cmd/flux-event.c 77.97% <0%> (ø) ⬆️
... and 3 more
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

hmm, code coverage is a tad low b/c of some circumstances where rootref was always set or not always set. Let me add some tests and see if we can do better.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

re-pushed with a few tests for additional coverage. There is one area in kvs_classic I (atleast for this round) decided not to cover b/c the combination of using flux_kvs_lookupat(), flux_kvs_lookup_dir(), and the classic functions flux_kvsdir_get/get_dir() is non-existent and we're getting rid of the classic functions soon too.

@chu11 chu11 force-pushed the chu11:issue1970 branch 2 times, most recently from 58a3258 to 4077c99 Jan 30, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

doh, chain lint bug in test, repushed

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

hit another SIGTERM like in #1949:

ERROR: t2202-job-manager.t - exited with status 137 (terminated by signal 9?)

but no "not cleanly shutdown" message, which is strange. Restarting.

chu11 added 7 commits Jan 29, 2019
A lookupat is passed a root reference, therefore it should not
be concerned with a namespace.

Adjust code in modules/kvs to not require a namespace to be specified
when a lookup contains a root reference.

Update tests and documentation accordingly.

Fixes #1970
For convenience, flux_kvs_lookupat() would call flux_kvs_lookup()
if the at reference was NULL.  Do not allow this, as it can
lead to lead to inconsistencies in the future if namespaces
are involved.

Adjust callers appropriately.
@chu11 chu11 force-pushed the chu11:issue1970 branch from 4077c99 to bbd148e Jan 31, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2019

rebased

@garlick

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

Looks good, and the missing coverage is mostly difficult to hit error paths. Thanks!

@garlick garlick merged commit 5f134f5 into flux-framework:master Feb 1, 2019
2 of 3 checks passed
2 of 3 checks passed
codecov/patch 76% of diff hit (target 80%)
Details
codecov/project 80.04% (+0.04%) compared to 096e89f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.