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: Support namespace prefix in key passed in by user #1423

Merged
merged 7 commits into from Apr 5, 2018

Conversation

4 participants
@chu11
Copy link
Contributor

chu11 commented Apr 3, 2018

This is the first half of the big patches to support namespace prefixes in issue #1341.

The first patches in this series are to support parsing of namespace prefixes. Then the support is added into the internal lookup and kvstxn APIs. Then finally some unit tests are added.

Note that I know I did not add documentation for namespace prefixes. I will do that later on as #1341 is completed.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 3, 2018

Hit a

PASS: t0015-cron.t 21 - module load with sync
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

restarting that build.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 3, 2018

Coverage Status

Coverage increased (+0.03%) to 78.89% when pulling 88547ec on chu11:issue1341-part4 into e0f968f on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 3, 2018

Codecov Report

Merging #1423 into master will increase coverage by 0.1%.
The diff coverage is 85.89%.

@@            Coverage Diff            @@
##           master    #1423     +/-   ##
=========================================
+ Coverage   78.54%   78.65%   +0.1%     
=========================================
  Files         163      163             
  Lines       30008    30082     +74     
=========================================
+ Hits        23571    23662     +91     
+ Misses       6437     6420     -17
Impacted Files Coverage Δ
src/modules/kvs/kvs.c 66.14% <100%> (+0.81%) ⬆️
src/modules/kvs/kvstxn.c 78.59% <77.77%> (-0.06%) ⬇️
src/modules/kvs/lookup.c 80.67% <78.94%> (+0.89%) ⬆️
src/modules/kvs/kvs_util.c 90% <81.48%> (-10%) ⬇️
src/common/libflux/request.c 88.46% <0%> (-1.29%) ⬇️
src/common/libkvs/kvs_watch.c 90.98% <0%> (-0.43%) ⬇️
src/common/libflux/handle.c 83.91% <0%> (+0.24%) ⬆️
src/common/libflux/message.c 81.25% <0%> (+0.35%) ⬆️
src/common/libutil/dirwalk.c 95% <0%> (+0.71%) ⬆️
... and 3 more

chu11 added some commits Mar 15, 2018

modules/kvs: Add namespace prefix helper function
In preparation for namespace prefix support, add helper function
to check for and parse keys with a namespace prefix.
modules/kvs: Parse namespace prefix in lookup path
In internal lookup API, parse a namespace prefix if specified by
the user in their lookup path.

@chu11 chu11 force-pushed the chu11:issue1341-part4 branch from 5bd5fc8 to c4c5a51 Apr 4, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 4, 2018

re-pushed with changes discussed in #1341. Namely, eliminate the support of "chaining" of namespaces. By doing so, a lot of code is simplified. Now when kvs_namespace_prefix() is called, it will check multiple corner cases, including if a namespace was specified twice. EINVAL is returned as necessary.

Since I had to refactor a nice chunk of code in this area, I went ahead and began support of #1421 as well. So now the namespace format of ns:X/ is used.

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

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

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 5, 2018

Would the strdups of chunks of key be avoidable if the fully qualified key was passed around with a "namespace length" value (0 for no namespace, number of characters in the namespace prefix o/w? Then strncmp could be used to compare namespaces, and addition could be used to get the key suffix?

@chu11 chu11 force-pushed the chu11:issue1341-part4 branch from c4c5a51 to ed83f3f Apr 5, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 5, 2018

I see what you're thinking. Some strdups could be avoided, but at several points, I need the namespace in its own string buffer, b/c it gets passed around (i.e. lookup namespace, pass namespace in rpc, etc.). So if I didn't strdup, I'd be copying into a buffer anyways.

Perhaps minimizing the strdups is something that could be done as an optimization later on, through a alternate kvs_namespce_prefix_pointers() or something function.

On this note, I noticed one place where kvs_namespace_prefix() doesn't need to parse the key_suffix. I went ahead and adjusted this, and squashed it since it was so tiny.

chu11 added some commits Mar 23, 2018

modules/kvs: Parse namespace prefix in txns
In commit/fence requests, parse and recognize alternate namespace
specified via namespace prefix.

If user tries to cross namespaces within commit/fence requests,
return EINVAL.

@chu11 chu11 force-pushed the chu11:issue1341-part4 branch from ed83f3f to 88547ec Apr 5, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 5, 2018

Just remembered, in json_pack(), s# or s% can be used to specify a string with a length. So that's perhaps one way to remove a strdup in a future optimization.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Apr 5, 2018

That was just a suggestion - if you're happy with this as is, I'm happy. LMK and I'll push the button.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Apr 5, 2018

I'm good with it.

@garlick garlick merged commit af89104 into flux-framework:master Apr 5, 2018

4 checks passed

codecov/patch 85.89% of diff hit (target 78.54%)
Details
codecov/project 78.65% (+0.1%) compared to e0f968f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 78.89%
Details

multi-user parallel jobs automation moved this from In progress to Done Apr 5, 2018

@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.