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: re-work kvs.sync #2016

Merged
merged 3 commits into from Feb 15, 2019

Conversation

@chu11
Copy link
Contributor

commented Feb 14, 2019

The last vestige of watch in modules/kvs was the use of the "watchlist" by kvs.sync. I considered leaving the code there b/c it was still useful by kvs.sync, but the implementation was inefficient. It would continually run through the whole watchlist (err "synclist") on every setroot event.

Instead, I created a new internal lib called "kvssync", that is a very simplified version of the former "watchlist" just for kvs.sync. The "synclist" is sorted by the sequence number people are waiting on. So everytime a setroot even occurs, you only traverse the list as far as you need to to alert syncers.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

Only travis error appears to be #2015, so gotta fix that first.

@chu11 chu11 force-pushed the chu11:rework_kvs_sync branch from 68221ba to 92b7e9b Feb 14, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

rebased

@garlick

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

Nice optimization!

I had a quick spin through this and it looks good. Coverage report will be good to see, just to make sure the new code is getting used.

@grondo

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

Would be nice to get all these kvs PRs merged in a row. If you don't mind closing issues by hand we could mergify them all, but up to you!

I leave final approval to @garlick

Copy link
Member

left a comment

No coverage report yet, but I think this can go in if it doesn't come back with anything @chu11 feels needs to be addressed.

@chu11 chu11 force-pushed the chu11:rework_kvs_sync branch from 92b7e9b to dae0c19 Feb 15, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

I saw a 85.71% diff coverage, woo-hoo! Only notable diff coverage miss was I didn't test two kvs.sync requests with the same version number being waited on (it's a miss in the sort comparison).

Added a dumb unit test to cover that path. Rebased & re-pushed.

chu11 added 3 commits Feb 6, 2019
Rename to last_update_epoch, to more accurately reflect what this
variable is used for.
As kvs watchers no longer exist in the kvs module, remove the old
kvs watchlist code that still existed due to its use within kvs.sync.

Replace the kvs.sync mechanism with a new internal lib called "kvssync"
that implements a very simple sync mechanism.  All sync-waiters are added
to a sorted list on kvsroot.  When a setroot() has occurred, responses
are only sent to those waiters at the front of the list.
@chu11 chu11 force-pushed the chu11:rework_kvs_sync branch from dae0c19 to 4328447 Feb 15, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Feb 15, 2019

Codecov Report

Merging #2016 into master will increase coverage by 0.04%.
The diff coverage is 87.01%.

@@            Coverage Diff             @@
##           master    #2016      +/-   ##
==========================================
+ Coverage   80.53%   80.57%   +0.04%     
==========================================
  Files         178      179       +1     
  Lines       28727    28779      +52     
==========================================
+ Hits        23135    23189      +54     
+ Misses       5592     5590       -2
Impacted Files Coverage Δ
src/modules/kvs/kvs.c 65.47% <70%> (+0.95%) ⬆️
src/modules/kvs/kvsroot.c 71.66% <75%> (ø) ⬆️
src/modules/kvs/kvssync.c 90.47% <90.47%> (ø)
src/common/libutil/aux.c 90.74% <0%> (-3.71%) ⬇️
src/modules/connector-local/local.c 73.62% <0%> (-1.04%) ⬇️
src/broker/modservice.c 79.8% <0%> (ø) ⬆️
src/common/libflux/message.c 81.51% <0%> (+0.12%) ⬆️
@garlick garlick merged commit f39d34f into flux-framework:master Feb 15, 2019
4 checks passed
4 checks passed
Mergify — Summary 1 potential rule
Details
codecov/patch 87.01% of diff hit (target 80.53%)
Details
codecov/project 80.57% (+0.04%) compared to 7af29ea
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.