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: remove watch handlers & api #2011

Merged
merged 7 commits into from Feb 14, 2019

Conversation

@chu11
Copy link
Contributor

commented Feb 13, 2019

With the wreck subsystem being removed, we can begin removing a lot of older stuff. We start by removing watch. This removes almost all of it. There is a bit of watching code in modules/kvs that is used by kvs.sync. I have a PR that re-does how kvs.sync works, which will be a follow up to this PR.

@grondo

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

I'm sure there is a good one, but any reason why flux kvs watch couldn't be redone using flux_kvs_lookup instead of requiring a --watch option for flux kvs get? A separate flux kvs watch command seems more user-friendly to me, but then again I haven't thought about it much, and also there might not be many uses cases for watch from the cmdline anyway.

Just wanted to bring itup.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

I'm sure there is a good one, but any reason why flux kvs watch couldn't be redone using flux_kvs_lookup instead of requiring a --watch option for flux kvs get?

I hadn't really thought about it much. I suppose @garlick originally added it to flux kvs get b/c it goes through the "lookup" interface instead of the "watch" interface?

It's worth mentioning that it would be hard / difficult to perfectly map the old "watch" to the "get --watch". A combination of --waitcreate and --pedantic would have to be specified by users to have similar effects.

@garlick

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

That might be nice, and may simplify the code in get a bit, since it shouldn't need to start the reactor and wait for multiple responses.

I wouldn't worry about matching the exact semantics of the old watch. I think we've moved on at this point.

@chu11 chu11 force-pushed the chu11:remove_watch branch from 64decc9 to 4e95c4d Feb 13, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

hmmm. We hit our connector.local Broken Pipe friend again (#1977, #1641, #1386, #542). A tad concerning b/c I did modify watch_disconnect in this PR. But I'm skeptical it's related to this.

2019-02-13T22:02:47.667891Z connector-local.err[0]: send kvs-watch.lookup response to client 7AFDC: Broken pipe
ok 1 - kvs watcher gets disconnected on client exit
PASS: t1103-apidisconnect.t 1 - kvs watcher gets disconnected on client exit
not ok 2 - multi-node kvs watcher gets disconnected on client exit
FAIL: t1103-apidisconnect.t 2 - multi-node kvs watcher gets disconnected on client exit
#	
#		${FLUX_BUILD_DIR}/t/kvs/watch_disconnect
#	
# failed 1 among 2 test(s)

Did notice a typo a log msg, so re-pushed.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

hmmm. Hit the connector.local Broken Pipe issue three more times. I'm guessing I screwed something up.

chu11 added 7 commits Feb 5, 2019
Refactor to use new lookup watch interface kvs-watch module over old
watch interface.  In transactionmerge, remove timeout watcher in favor
of future then continuation timeout.
Closes #1980
@chu11 chu11 force-pushed the chu11:remove_watch branch from 4e95c4d to 86572ad Feb 14, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

re-pushed. I think I had to set WAITCREATE on the watch_disconnect test, otherwise the test can race with a ENOENT being sent back to the caller. TBH, I'm shocked the test regularly passed on catalyst or that it often passed in travis. Glad it was caught.

@codecov-io

This comment has been minimized.

Copy link

commented Feb 14, 2019

Codecov Report

Merging #2011 into master will decrease coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2011      +/-   ##
==========================================
- Coverage   80.39%   80.32%   -0.08%     
==========================================
  Files         180      179       -1     
  Lines       29394    28898     -496     
==========================================
- Hits        23632    23211     -421     
+ Misses       5762     5687      -75
Impacted Files Coverage Δ
src/modules/kvs/kvs.c 64.52% <ø> (-1.82%) ⬇️
src/cmd/flux-kvs.c 82.26% <ø> (-0.81%) ⬇️
src/common/libutil/msglist.c 74.39% <0%> (-4.88%) ⬇️
src/common/libutil/aux.c 90.74% <0%> (-3.71%) ⬇️
src/common/libflux/handle.c 83.71% <0%> (-0.46%) ⬇️
src/modules/kvs/lookup.c 80.6% <0%> (-0.39%) ⬇️
src/common/libflux/message.c 81.51% <0%> (-0.13%) ⬇️
src/common/libflux/future.c 87.86% <0%> (+0.32%) ⬆️
Copy link
Member

left a comment

Feels pretty good to get rid of that old watch code!

This LGTM @chu11. On @grondo's suggestion about flux kvs watch, perhaps we could open a separate issue to discuss re-adding it. It seems like a good idea, although I also wonder if it will get used enough to make the effort worthwhile. I would expect "get" with waitcreate to be more useful on the command line, but the recurring watch I'm not so sure.

I wonder if some of the tests modified by this PR could simplified/clarified a bit, but I don't think that's related to this PR and we have other priorities.

@garlick

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

@grondo if you concur on my flux kvs watch point, freel free to add merge-when-passing.

@grondo

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

if you concur on my flux kvs watch point, freel free to add merge-when-passing.

Yeah, I agree with your point about flux kvs watch.

I think we have to pause using mergify to merge our PRs because it doesn't seem to be able to auto-close issues mentioned in the PR and issues.

I'll push the merge button manually and verify it auto-closes the issues @chu11 has mentioned in his commits.

@grondo grondo merged commit 872fad3 into flux-framework:master Feb 14, 2019
4 checks passed
4 checks passed
Mergify — Summary 1 potential rule
Details
codecov/patch Coverage not affected when comparing 16c1e9d...86572ad
Details
codecov/project 80.32% (-0.08%) compared to 16c1e9d
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
4 participants
You can’t perform that action at this time.