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: Refactor prep work for namespace prefix support #1390

Merged
merged 12 commits into from Mar 28, 2018

Conversation

5 participants
@chu11
Copy link
Contributor

chu11 commented Mar 27, 2018

This PR does a lot of refactoring on the internal kvs lookup API. The two major accomplishments of this PR are:

  1. Allow kvs namespaces to be looked up within the lookup API itself.

  2. When a namespace is missing/not available, inform callers that it is missing and has to be brought in.

Then of course there are tons of unit tests along the way.

chu11 added some commits Mar 9, 2018

modules/kvs: Add more data to lookup_t
Refactor lookup_create() to take a kvsroot_mgr_t, rolemask, and
owner and store it in a lookup handle.

This code only does the refactoring to take these new parameters,
internally they are not yet used.
modules/kvs: Lookup root ref via namespace
In lookup_create(), instead of always requiring the user to
specify the root reference, instead make it optional.  If the
user does not specify it, find it via the namespace.

In this patch, it is assumed the namespace specified by the
user always exists in the kvsroot_mgr_t.

Update unit tests accordingly.
modules/kvs: Return lookup enum instead of bool
Refactor lookup() to return an enum indicating the exact
thing that occured within the lookup (i.e. error occurred), instead
of returning a bool but requiring the user to check lookup_get_errnum()
all the time.
modules/kvs: Refactor walk() to return enum
Refactor the internal walk() function to return an enum indicating
the specific result of the lookup instead of just returning
a bool and requiring the user to check the errnum.
modules/kvs: Add unit test for double lookup call
Add test to cover situation in which user calls lookup() twice
when an error occurs.
modules/kvs: Refactor when kvs root is looked up
In internal lookup API, move where root reference is looked up
via namespace from within create time to during lookup time (i.e
from lookup_create() to lookup()).

Adjust unit test appropriately.
modules/kvs: Allow lookup to stall on namespace
Allow internal lookup API to stall if a namespace it desires is
missing from the kvsroot_mgr_t data structure.

Support new function lookup_missing_namespace() to inform user
what namespace is missing so it can be loaded.
modules/kvs: Add namespace removed checks
In internal lookup API, add checks on a replay to ensure
that the namespace the user is working on still exists.
modules/kvs: Utilize lookup API namespace lookups
Adjust core lookup and watch code to take advantage of recent
changes allowing for namespace stalls in the lookup API.  Remove
now unnecessary calls to lookup kvs namespaces.

@chu11 chu11 added this to To do in multi-user parallel jobs via automation Mar 27, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 27, 2018

Coverage Status

Coverage increased (+0.05%) to 78.855% when pulling ce689f4 on chu11:issue1341-part2 into 7fd643e on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 27, 2018

Codecov Report

Merging #1390 into master will increase coverage by 0.04%.
The diff coverage is 82.01%.

@@            Coverage Diff             @@
##           master    #1390      +/-   ##
==========================================
+ Coverage   78.49%   78.54%   +0.04%     
==========================================
  Files         162      162              
  Lines       29724    29778      +54     
==========================================
+ Hits        23332    23389      +57     
+ Misses       6392     6389       -3
Impacted Files Coverage Δ
src/modules/kvs/kvs.c 65.33% <100%> (+0.47%) ⬆️
src/modules/kvs/lookup.c 79.6% <75%> (+0.99%) ⬆️
src/common/libflux/message.c 81.25% <0%> (-0.36%) ⬇️
src/broker/overlay.c 74.14% <0%> (-0.32%) ⬇️
src/common/libflux/future.c 88.78% <0%> (+0.46%) ⬆️
src/common/libutil/dirwalk.c 94.28% <0%> (+0.71%) ⬆️
src/modules/connector-local/local.c 74.38% <0%> (+1.43%) ⬆️

@chu11 chu11 moved this from To do to In progress in multi-user parallel jobs Mar 27, 2018

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Mar 28, 2018

I don't feel I'm qualified to comment on this PR, but since @garlick is out and to keep your work moving forward, I thought I'd take a quick look to see if this could be merged.

My only comments are that the lookup() function itself is getting pretty hefty. Especially for the code blocks under the case statements for the lookup states, would it be more clear if some of that moved to their own functions? (I didn't look close enough to see if that would be unwieldy for some reason)

Also, the lookup_process_t seems more like a state enumeration? It seemed like it might be clearer if it was named lookup_state_t, but then I assume maybe I'm just misreading the code.

Otherwise, I would be happy to merge this now if you'd like. If there are other comments by @garlick later those could always be fixed up in a future PR.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Mar 28, 2018

Just did a very quick pass through this and didn't see anything big to bring up beyond @grondo's comments. @grondo when you're happy please go ahead and press the button.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Mar 28, 2018

My comments were minor and could always be fixed up in future refactoring PRs. In the interest of moving things along I'll merge.

@grondo grondo merged commit cdc9e9b into flux-framework:master Mar 28, 2018

4 checks passed

codecov/patch 82.01% of diff hit (target 78.49%)
Details
codecov/project 78.54% (+0.04%) compared to 7fd643e
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 78.855%
Details

multi-user parallel jobs automation moved this from In progress to Done Mar 28, 2018

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Mar 28, 2018

Cool. OK @chu11 please keep in mind that maybe that some of the internal KVS API implementations might need a review/refactor pass. I hate to keep pushing for more features and not give the impression that taking the time to do that sort of thing isn't available. We should make time for it as we go.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 28, 2018

Yeah, some of the code is getting hefty. I've had to functionize a few things here and there as I was going along. As I've continued on with the work, I think I'll have to do more of these things.

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.