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 FLUX_KVS_WATCH_FULL #1848

Merged
merged 4 commits into from Nov 20, 2018

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Nov 19, 2018

This implements FLUX_KVS_WATCH_PEDANTIC as discussed in #1653.

Note that I still dislike the option being called "pedantic" and am wondering what a better name is. I originally wrote this using the word "classic", but that felt wrong, as "classic" implies the API would stay the same, which is not the case.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 20, 2018

Fantastic! I'll have a closer look tomorrow, but I'm impressed it could be done with so little code. Does PEDANTIC imply WAITCREATE?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 20, 2018

No, it doesn't imply waitcreate. It'll still error out on ENOENT just like the old watch code.

Now that you mention it, I should probably add some tests to ensure --pedantic and --waitcreate work together (which I believe they should).

@chu11 chu11 force-pushed the chu11:issue1653 branch from 19e645c to 943abea Nov 20, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 20, 2018

just re-pushed, adding one more test using --pedantic && --waitcreate (went ahead and squashed it).

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 20, 2018

I'm having a tough time thinking of good flag names. Maybe FLUX_KVS_WATCH_CHECKALL instead of PEDANTIC?

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 20, 2018

Codecov Report

Merging #1848 into master will decrease coverage by <.01%.
The diff coverage is 81.08%.

@@            Coverage Diff             @@
##           master    #1848      +/-   ##
==========================================
- Coverage   79.89%   79.88%   -0.01%     
==========================================
  Files         196      196              
  Lines       35267    35300      +33     
==========================================
+ Hits        28176    28201      +25     
- Misses       7091     7099       +8
Impacted Files Coverage Δ
src/cmd/flux-kvs.c 83.48% <100%> (+0.03%) ⬆️
src/common/libkvs/kvs_lookup.c 81.97% <100%> (+0.31%) ⬆️
src/modules/kvs-watch/kvs-watch.c 76.23% <78.12%> (+0.16%) ⬆️
src/common/libflux/response.c 79.62% <0%> (-1.24%) ⬇️
src/common/libflux/message.c 81.15% <0%> (-0.37%) ⬇️
src/modules/kvs/kvs.c 65.8% <0%> (+0.15%) ⬆️
src/cmd/flux-module.c 85.63% <0%> (+0.27%) ⬆️
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 20, 2018

FLUX_KVS_WATCH_CHECKALL

This sounds more like something that would apply to a directory? Like "check everything here"?

How about "COMPARE_ALWAYS" (or "ALWAYS_COMPARE")? The problem is will that be confusing given the potential FLUX_KVS_WATCH_UNIQ you suggested in #1847? Would something like "ALWAYS_COMPARE" imply "UNIQ"?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 20, 2018

Oh wait, is "CHECKALL" short for something like "check after all setroots"? I had read it differently earlier. Maybe "CHECK_ALWAYS"?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 20, 2018

Users of the API may not know anything about how kvs watch is implemented, so the flags do not need or shouldn't be tied to how they are changing the implementation, but rather their intended effect.

This is just my opinion as an outsider/user of the API, but what about something simpler like FLUX_KVS_WATCH_FULL to indicate the flag will trigger a response in more conditions than the default "lightweight" watch?

I also don't mind FLUX_KVS_WATCH_CHECKALL, but the CHECK seems redundant and this could be shortened to FLUX_KVS_WATCH_ALL?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 20, 2018

FLUX_KVS_WATCH_FULL

I like this option name. Or with a slightly different twist, "FLUX_KVS_WATCH_HEAVY", which indicates it's the more heavyweight option.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 20, 2018

I think my vote would be for FLUX_KVS_WATCH_FULL rather than HEAVY. HEAVY again is more about implementation than semantics.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 20, 2018

Ok, lets go with FULL.

@chu11 chu11 force-pushed the chu11:issue1653 branch from 943abea to cc8e301 Nov 20, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 20, 2018

just re-pushed with pedantic changed to full. All squashed since it was just a search & replace really. A few minor English edits as a result too.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 20, 2018

Don't forget to change PR title to reflect flag name change.

@chu11 chu11 changed the title kvs: Support FLUX_KVS_WATCH_PEDANTIC kvs: Support FLUX_KVS_WATCH_FULL Nov 20, 2018

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 20, 2018

I was just pondering if there was a way we could avoid fetching the value of a watched key on each commit. The old heavyweight watch only needed to get to the dirent level internally to decide if a value changed...

Just fetching the dirent and then if it changes, fetching the value seems unsatisfying.

I can't help but wonder if the kvs module (as opposed to kvs-watch) might be able to provide some new method for internal use by kvs-watch that would be trivial to implement and reduce the overhead here.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 20, 2018

This looks good, and I'd be OK with merging and revisiting the issue I raised about overhead later on as a performance optimization.

It seems like we are within striking distance of being able to get rid of the old implementation!

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 20, 2018

I was just pondering if there was a way we could avoid fetching the value of a watched key on each commit. The old heavyweight watch only needed to get to the dirent level internally to decide if a value changed...

The current kvs.watch does a lookup (via the lookup api) to get the value and then internally does a json_equal() comparison to check for differences. Are you thinking about an implementation from way way back?

Either way, we can create a new issue to discuss performance improvements. This will probably also involve discussions of preserving the read-your-writes invariant and potential future "merging" of kvs and kvs-watch.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 20, 2018

Oh, I assumed it was the RHS of the dirent (which could be the whole value, or an array of blobrefs). Sorry, never mind, let's get this in and discuss performance elsewhere.

Ready to merge?

p.s. #1840 is awaiting your input

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 20, 2018

yup, ready to merge. I'll rebase and re-push. Didn't know #1840 was waiting on me, I'll take a look this afternoon.

chu11 added some commits Nov 19, 2018

man1/flux-kvs: Add clarification on get --watch
Note that the --watch option watches for all times a key is written
to, not if it changes.
modules/kvs-watch: Support FLUX_KVS_WATCH_FULL
Support new flag that supports "kvs.watch" semantics.  In particular,
instead of watching for specific writes to a key, lookup a key on every
setroot change and see if the value has changed.

Fixes #1653

@chu11 chu11 force-pushed the chu11:issue1653 branch from cc8e301 to baf5d50 Nov 20, 2018

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 20, 2018

Great job @chu11.

@garlick garlick merged commit 3f6a417 into flux-framework:master Nov 20, 2018

3 checks passed

codecov/patch 81.08% of diff hit (target 79.89%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +1.18% compared to 8dd15c9
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.