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 FLUX_KVS_WATCH_APPEND #2056

Merged
merged 9 commits into from Mar 1, 2019

Conversation

@chu11
Copy link
Contributor

commented Feb 28, 2019

Support a new watch flag that will only respond with data appended to a key, not the full value of the key (#2054).

Notes:

  • If an append does not occur, such as the key being overwritten by a new value, EINVAL is sent back to the user. Better errno option?

  • B/c it was easier, I elected to implement this watch via the key VALUES, not the valref treeobj array. Because of this fact, appends can be faked. e.g. the watch responses from

  flux kvs put key="a"
  flux kvs put --append key="b"
  flux kvs put --append key="c"

would be identical to watch results from

  flux kvs put key="a"
  flux kvs put key="ab"
  flux kvs put key="abc"

I felt this is the correct behavior to support. Error in the watch would occur if a user instead did:

  flux kvs put key="a"
  flux kvs put key="b"

b/c it's clearly not an append.

I fixed up some stupid things along the way, including #2048.

@chu11 chu11 force-pushed the chu11:issue2054 branch 2 times, most recently from dc8af24 to 85e1210 Feb 28, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

good thing for coverage tests. Missed some coverage, realized I had a missing test, and that missing test failed b/c I had a bug. Re-pushed.

@garlick

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Nice that this isn't a lot of code!

I wonder if storing the full "value" (cumulative) in the watch state (in kvs-watch module) is needed? If there were a lot of these going at once, that could eat up quite a lot of memory?

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

I wonder if storing the full "value" (cumulative) in the watch state (in kvs-watch module) is needed? If there were a lot of these going at once, that could eat up quite a lot of memory?

Yeah, it's the one thing I was a bit iffy on. Without it, the only check I would do is on data length. So something like:

  flux kvs put key="a"
  flux kvs put key="bb"

would return a watch response of "a" and "b", since the length is ok. But that seems really wrong.

We could of course do something clever like hash the value and see if the hash is identical? So that guarantees us a max storage? In fact, something clever like that could be used for FLUX_KVS_WATCH_FULL and FLUX_KVS_WATCH_UNIQ as well.

@garlick

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

What about watching the treeobj, and just keeping an offset into the blobref array? Stashing the most recently returned blobref might be sufficient to detect most non-append updates? (And again, I don't think catching all possible cases of that is strictly necessary...)

@garlick

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

What about watching the treeobj, and just keeping an offset into the blobref array

That was probably a bogus suggestion since you'd then have to go fetch the blobs, duplicating code in the kvs that can do that with the benefit of a local cache.

@garlick

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Without it, the only check I would do is on data length

Might be OK.

Another idea (may not be a good one!): adding an offset to lookup-plus, to avoid fetching the same data over and over.

@chu11 chu11 force-pushed the chu11:issue2054 branch from 85e1210 to 8fadac7 Feb 28, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

That was probably a bogus suggestion since you'd then have to go fetch the blobs, duplicating code in the kvs that can do that with the benefit of a local cache.

Yeah, that was exactly the reason :-)

Another idea (may not be a good one!): adding an offset to lookup-plus, to avoid fetching the same data over and over.

I had considered making a lookup-plus-append kind of special RPC to handle a lot of the internal work, but it seemed like more work than was necessary. Given a previous treeobj for comparison, handle the lookup of new blobrefs to get the new value.

Another gotcha to recall. Multiple appends can happen in the same kvs transaction, so a watch-append response could contain data from multiple blobrefs. This is one of the gotchas that made me go with the way I implemented it. (which now that I mention it, I should have added a test for that, added that test and re-pushed).

Without it, the only check I would do is on data length
Might be OK.

So do we want to go with it? I think the only thing is the language/doc of the feature would maybe have to be tweaked a bit. Data past the previous data's length is returned.

@garlick

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Multiple appends can happen in the same kvs transaction, so a watch-append response could contain data from multiple blobrefs. This is one of the gotchas that made me go with the way I implemented it. (which now that I mention it, I should have added a test for that, added that test and re-pushed).

I assumed this would be the case. Are you saying you worked around that somehow?

My feeling is let's go without checking that already-returned content hasn't changed, unless you feel strongly about retaining that test?

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

I assumed this would be the case. Are you saying you worked around that somehow?

No, just wanted to note that by comparing key values this is "automatic", but if we did it with treeobject valref indexes, there'd be some additional trickiness.

My feeling is let's go without checking that already-returned content hasn't changed, unless you feel strongly about retaining that test?

Nah, I just think some of the documentation needs to be adjusted to note expectations.

@chu11 chu11 force-pushed the chu11:issue2054 branch 2 times, most recently from c4b63c7 to 2013d0c Feb 28, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

rebased & re-pushed with the data comparison removed and a minor tweak to the manpage noting that only data length is looked at.

Copy link
Member

left a comment

Looks good. Just a couple minor comments then this can go in.

One minor point: it seems like the number of flux kvs get options is becoming large and it's probably less obvious than it should be which options only apply in the "watch" case. We should probably consider @grondo's suggestion of splitting out a watch subcommand at some point

committed transaction and the key has been appended to. The response
will only contain the additional appended data. Note that only data
length is considered for appends and no guarantee is made that prior
data hasn't have been overwritten.

This comment has been minimized.

Copy link
@garlick

garlick Feb 28, 2019

Member

There's an extra "have" in this sentence.

@@ -267,6 +268,15 @@ static int handle_initial_response (flux_t *h,
|| (w->flags & FLUX_KVS_WATCH_UNIQ))
w->prev = json_incref (val);

if (w->flags & FLUX_KVS_WATCH_APPEND) {

This comment has been minimized.

Copy link
@garlick

garlick Feb 28, 2019

Member

Double parens are usually advised when testing a bit value inside an if.

json_decref (new_val);
}

w->responded = true;

This comment has been minimized.

Copy link
@garlick

garlick Feb 28, 2019

Member

set inside !w->responded conditional leg?

This comment has been minimized.

Copy link
@chu11

chu11 Mar 1, 2019

Author Contributor

ahh, i copied this from the handle_compare_response() function, should move it in there too.

free (new_data);
w->append_offset = new_offset;

if (flux_respond_pack (h, w->request, "{ s:O }", "val", new_val) < 0) {

This comment has been minimized.

Copy link
@garlick

garlick Feb 28, 2019

Member

use little 'o' (steals reference) and eliminate json_decref in success case?

@chu11 chu11 force-pushed the chu11:issue2054 branch from 2013d0c to dd6ada7 Mar 1, 2019
chu11 added 9 commits Feb 27, 2019
Mention FLUX_KVS_WATCH_UNIQ when appropriate.
Do not output ellipsis if there is no text after a newline break.

Fixes #2048
Add FLUX_KVS_WATCH_UNIQ and FLUX_KVS_WATCH_FULL descriptions.
Fix incorrect tabbing on bracket and move setting of flag
inside branch.
Support flag which will support watching a key and only reporting
data which has been appended to it.

Fixes #2054
@chu11 chu11 force-pushed the chu11:issue2054 branch from dd6ada7 to 73c3914 Mar 1, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

rebased & re-pushed with changes suggested by @garlick

@codecov-io

This comment has been minimized.

Copy link

commented Mar 1, 2019

Codecov Report

Merging #2056 into master will decrease coverage by <.01%.
The diff coverage is 65.78%.

@@            Coverage Diff             @@
##           master    #2056      +/-   ##
==========================================
- Coverage   80.41%   80.41%   -0.01%     
==========================================
  Files         191      191              
  Lines       30252    30287      +35     
==========================================
+ Hits        24328    24354      +26     
- Misses       5924     5933       +9
Impacted Files Coverage Δ
src/common/libkvs/kvs_lookup.c 83.52% <ø> (ø) ⬆️
src/cmd/flux-kvs.c 82.38% <100%> (+0.06%) ⬆️
src/modules/kvs-watch/kvs-watch.c 78.78% <61.76%> (-1.4%) ⬇️
src/common/libflux/message.c 81.15% <0%> (-0.25%) ⬇️
src/cmd/flux-module.c 83.72% <0%> (-0.24%) ⬇️
src/broker/modservice.c 79.8% <0%> (+0.96%) ⬆️
src/common/libutil/veb.c 98.85% <0%> (+3.42%) ⬆️
@garlick

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Great! Thanks!

@garlick garlick merged commit e59d93b into flux-framework:master Mar 1, 2019
3 of 4 checks passed
3 of 4 checks passed
codecov/patch 65.78% of diff hit (target 80.41%)
Details
Mergify — Summary 1 potential rule
Details
codecov/project 80.41% (-0.01%) compared to af26b4f
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
Projects
None yet
3 participants
You can’t perform that action at this time.