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 ability to list namespaces #1336

Merged
merged 5 commits into from Feb 13, 2018

Conversation

4 participants
@chu11
Copy link
Contributor

chu11 commented Feb 13, 2018

Support a simple iterator to allow users to list the KVS namespaces that exist, along with the owner and current flags. The flux kvs namespace-list option would list something like this:

NAMESPACE             OWNER      FLAGS
primary                8556 0x00000000
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Feb 13, 2018

hmmm, a stalled build, last line was

PASS: ../t/t9990-python-tests.t 44 test.request.TestRequestMethods.test_null_payload

this one appears to be new. Just jotting it down, will restart build.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 13, 2018

Coverage Status

Coverage increased (+0.09%) to 78.632% when pulling 1f5f07b on chu11:kvsnamespacelist into 04d2409 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 13, 2018

Codecov Report

Merging #1336 into master will increase coverage by 0.09%.
The diff coverage is 76.19%.

@@            Coverage Diff            @@
##           master   #1336      +/-   ##
=========================================
+ Coverage   78.21%   78.3%   +0.09%     
=========================================
  Files         156     156              
  Lines       28321   28405      +84     
=========================================
+ Hits        22151   22243      +92     
+ Misses       6170    6162       -8
Impacted Files Coverage Δ
src/modules/kvs/kvs.c 65.56% <62.96%> (-0.07%) ⬇️
src/cmd/flux-kvs.c 80.91% <75%> (-0.11%) ⬇️
src/common/libkvs/kvs.c 90.52% <84.44%> (-5.48%) ⬇️
src/common/libkvs/kvs_watch.c 90.55% <0%> (-0.86%) ⬇️
src/common/libflux/msg_handler.c 86.64% <0%> (-0.37%) ⬇️
src/broker/overlay.c 73.88% <0%> (-0.32%) ⬇️
src/common/libflux/message.c 81.01% <0%> (-0.24%) ⬇️
src/common/libflux/request.c 88.46% <0%> (ø) ⬆️
src/common/libflux/future.c 88.78% <0%> (ø) ⬆️
src/bindings/lua/flux-lua.c 81.16% <0%> (+0.08%) ⬆️
... and 8 more
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 13, 2018

I'd probably flatten the new service method name, since the extra period implies a module hierarchy (see intro in RFC 5), e.g. use kvs.listns or similar.

It might be a good idea to have a name column wide enough for a uuid in RFC 4122 ascii form, e.g.

f81d4fae-7dec-11d0-a765-00a0c91e6bf6

since it's handy to create a unique KVS namespace that way, if no other source of uniqueness is available.

@garlick garlick added the in progress label Feb 13, 2018

@garlick garlick added this to To do in multi-user parallel jobs via automation Feb 13, 2018

@garlick garlick moved this from To do to In progress in multi-user parallel jobs Feb 13, 2018

chu11 added some commits Feb 9, 2018

common/libkvs: Support flux_kvs_namespace_list()
Also support iterator functions to iterate through namespaces.

Add unit tests & documentation appropriately.
modules/kvs: Fix namespace-create corner case
If owner of created namespace is FLUX_USERID_UNKNOWN, store the
instance owner's userid as the owner.

@chu11 chu11 force-pushed the chu11:kvsnamespacelist branch from 9089f8f to 1f5f07b Feb 13, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Feb 13, 2018

Just re-pushed with spacing enough to handle uuid width.

Noticed coverage didn't hit 78%, so I reworked some code in the kvs module. There is no need to handle the "zero namespaces" path, since json_pack() will do the smart thing with an empty array.

Changes are tiny and went ahead and squashed it all.

I'd probably flatten the new service method name

I see your point. How about we clean that up within the context of #1302 instead. Because I should probably correct that in a lot of places (kvs.stats.get, kvs.namespace.create, kvs.namespace.remove, kvs.setroot.*, etc.). I'm thinking underscores/dashes or something instead of periods.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 13, 2018

Sure that's fine by me.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Feb 13, 2018

Just checked this out and played with it a bit - nice work. Are you still trying to hit 78% or do you think it's ready to go?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Feb 13, 2018

I think its good to go. If I wrote a whole new C file test I think I could only eek out one more line of coverage, so doubt I'm going to get to 78%.

@garlick garlick merged commit 1b7d626 into flux-framework:master Feb 13, 2018

3 of 4 checks passed

codecov/patch 76.19% of diff hit (target 78.21%)
Details
codecov/project 78.3% (+0.09%) compared to 04d2409
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 78.632%
Details

multi-user parallel jobs automation moved this from In progress to Done Feb 13, 2018

@garlick garlick removed the in progress label Feb 14, 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.