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-watch: Support lookup with WAITCREATE flag and w/o need for WATCH flag #1870

Merged
merged 7 commits into from Dec 14, 2018

Conversation

Projects
None yet
3 participants
@chu11
Copy link
Contributor

chu11 commented Dec 14, 2018

Per discussion in #1828, this PR allows users to specify the WAITCREATE flag without specifying the WATCH flag. This allows users to do a basic lookup that waits for a key to be created before returning.

Given the minimal changes necessary to support this, I was initially hesitant to believe that I had done this correctly. But I think this is all that's necessary.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 14, 2018

Codecov Report

Merging #1870 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1870      +/-   ##
==========================================
- Coverage   80.09%   80.09%   -0.01%     
==========================================
  Files         196      196              
  Lines       34992    34998       +6     
==========================================
+ Hits        28028    28030       +2     
- Misses       6964     6968       +4
Impacted Files Coverage Δ
src/modules/kvs-watch/kvs-watch.c 78.17% <100%> (+0.09%) ⬆️
src/cmd/flux-kvs.c 83.27% <100%> (ø) ⬆️
src/common/libkvs/kvs_lookup.c 81.14% <100%> (-0.73%) ⬇️
src/common/libflux/handle.c 83.99% <0%> (-0.7%) ⬇️
src/common/libflux/message.c 81.64% <0%> (+0.12%) ⬆️
@garlick
Copy link
Member

garlick left a comment

Nice that this is such a compact change. I'll poke at it a bit but wanted to get a few comments in first.

Show resolved Hide resolved src/common/libkvs/kvs_lookup.c Outdated
Show resolved Hide resolved src/modules/kvs-watch/kvs-watch.c Outdated
Show resolved Hide resolved doc/man1/flux-kvs.adoc Outdated
Show resolved Hide resolved t/t1007-kvs-lookup-watch.t
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 14, 2018

This looks good. I tried running flux kvs get --waitcreate foo, then in another window, flux kvs put foo=bar, and that worked as expected.

Next I tried the same, but on a non-default namespace that didn't exist when the get was started, and it worked as expected.

So victory I think!

chu11 added some commits Dec 14, 2018

common/libkvs: Cleanup flag clears
Clear flags via FLUX_KVS_WATCH_FLAGS instead of each individual
flag.
common/libkvs: Rename FLUX_KVS_WATCH_WAITCREATE
Rename to FLUX_KVS_WAITCREATE.  In the future, it need not be
tied to a WATCH flag.
modules/kvs-watch: Support WAITCREATE w/o WATCH
Support the ability to specify WAITCREATE w/o WATCH.  This option
will effectively allow users to do a lookup and wait until a key
has been created.

Fixes #1828
cmd/flux-kvs: Support --waitcreate without --watch
Allow users to specify --waitcreate without --watch, effectively
allowing a basic lookup to wait for a key to be created.

Note that specifying --watch & --waitcreate is still supported.

@chu11 chu11 force-pushed the chu11:issue1828 branch from f39bc3f to 1598364 Dec 14, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 14, 2018

re-pushed and went ahead and squashed tiny fixes. Added some fixes for parens around bitwise ops that were missed in prior patches.

I didn't fix the test in discussion above, b/c I think it should be kept way it is. Unless I'm missing something about the suggestion.

@chu11 chu11 force-pushed the chu11:issue1828 branch from 1598364 to 0eecb1a Dec 14, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 14, 2018

repushed with the extra --waitcreate test suggested

t/kvs: Add get --waitcreate tests
Add tests calling flux kvs get --waitcreate without the
--watch option.

@chu11 chu11 force-pushed the chu11:issue1828 branch from 0eecb1a to 493e984 Dec 14, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 14, 2018

re-pushed, forgot some NO_CHAIN_LINTs (not sure how it passed before though).

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Dec 14, 2018

Nice! Thanks @chu11!

@garlick garlick merged commit bb53541 into flux-framework:master Dec 14, 2018

3 checks passed

codecov/patch 100% of diff hit (target 80.09%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +19.9% compared to d3a10d3
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
You can’t perform that action at this time.