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: Make getroot() asynchronous #1315

Merged
merged 6 commits into from Jan 15, 2018

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Jan 10, 2018

Per discussion in #1197, make getroot() call asynchronous.

In this PR, a struct getroot_handler was added to handle information needed in getroot continuation calls. Two design notes on this:

  1. The getroot call is needed in kvs.get, kvs.watch, kvs.sync, and kvs.fence. This collection of request handlers were different enough from each other that it seemed more work than would be reasonable to place the getroot call within the lookup interface, commit interface, and whatever I would do for kvs.sync. So I elected to handle getroot asynchronous-ness outside of those interfaces.

  2. This getroot handler code is 90% similar to what is in waitqueue.c code. But b/c of the abstraction of wait_t, I couldn't quite exploit using it. Refactoring a common "callback handler" set of functions could be done, but I punted on this for the time being.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jan 10, 2018

Might want to use the same function naming style as the adjacent "load" async calls, e.g. getroot_request_send() and getroot_completion(). Just a thought.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 11, 2018

Coverage Status

Coverage decreased (-0.04%) to 78.55% when pulling 67e5931 on chu11:kvsgetrootasync into a4d438a on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 11, 2018

Codecov Report

Merging #1315 into master will decrease coverage by 0.07%.
The diff coverage is 78.43%.

@@            Coverage Diff             @@
##           master    #1315      +/-   ##
==========================================
- Coverage   78.29%   78.21%   -0.08%     
==========================================
  Files         154      155       +1     
  Lines       28133    28189      +56     
==========================================
+ Hits        22026    22049      +23     
- Misses       6107     6140      +33
Impacted Files Coverage Δ
src/modules/kvs/kvs.c 65.47% <73.84%> (-0.06%) ⬇️
src/modules/kvs/waitqueue.c 84.46% <81.81%> (-0.98%) ⬇️
src/modules/kvs/msg_cb_handler.c 88.46% <88.46%> (ø)
src/modules/connector-local/local.c 72.95% <0%> (-1.44%) ⬇️
src/broker/module.c 83.79% <0%> (-1.4%) ⬇️
src/broker/content-cache.c 73.43% <0%> (-1.3%) ⬇️
src/common/libflux/future.c 88.31% <0%> (-0.47%) ⬇️
src/common/libflux/message.c 81.48% <0%> (-0.24%) ⬇️
... and 2 more
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 11, 2018

@garlick Ahh, good idea.

modules/kvs: Make getroot() asynchronous
Make getroot() kvs namespace initialization call asynchronous
instead of synchronous.  Involved is creation of callback data
structure to be used in response continuations.

@chu11 chu11 force-pushed the chu11:kvsgetrootasync branch from 67e5931 to c708ccb Jan 11, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 11, 2018

Coverage Status

Coverage decreased (-0.07%) to 78.519% when pulling c708ccb on chu11:kvsgetrootasync into a4d438a on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jan 12, 2018

I think your approach is reasonable @chu11. Thanks for pushing this through.

Would it make sense to move getroot() and related functions to their own .c file? It seems like there might be enough self-contained functionality for that to help with clarity. It may also make it easier to build unit test coverage.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jan 12, 2018

Possibly a kvsroot.c could include the root hash and functions for manipulating it too...

If we talked about this before and I'm not remembering the result, apologies in advance!

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 12, 2018

Ya know, the first time I looked at making a kvsroot.c it felt a tad more daunting. But now that I look through the code again (especially after PR #1314), it doesn't look quite as bad as before. Add in the common-ness of this PR with the code in waitqueue.c, the need appears to have increased.

I would agree another refactoring is in order.

chu11 added some commits Jan 12, 2018

modules/kvs: Refactor core KVS getroot_handler
Use new msg_cb_handler internal API instead of the getroot_handler
functions that were previously developed.
modules/kvs: Refactor waitqueue msg handler use
Use new msg_cb_handler internal API instead of prior msg handler
code.
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 13, 2018

Since this PR wasn't merged yet, I went ahead and created a new abstraction msg_cb_handler that can be used both in the core KVS file and waitqueue. A kvsroot abstraction will be for another day of course.

chu11 added some commits Jan 13, 2018

modules/kvs: Refactor wait_create_msg_handler
Change parameter inputs to be consistent to msg_cb_handler API.
@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 13, 2018

and I added a few cleanup patches along the way too.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 13, 2018

Coverage Status

Coverage decreased (-0.03%) to 78.561% when pulling 32396fe on chu11:kvsgetrootasync into a4d438a on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 13, 2018

Coverage Status

Coverage decreased (-0.04%) to 78.547% when pulling 32396fe on chu11:kvsgetrootasync into a4d438a on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jan 15, 2018

Apologies for the delay getting back to this!

There might be an opportunity to simplify this further by storing intermediate message processing state (such as a lookup_t) in the message itself using flux_msg_aux_set(), and then rather than storing the message handler and parameters it was called with when stalling, simply storing the message, and then re-queueing it in the handle with flux_requeue(RQ_HEAD) when it's ready to restart. (OK, we would have to add a flux_requeue_nocopy() that doesn't copy the message and thus drop any annotations, but that's easy enough.) That would enter the reactor more frequently too, which might improve responsiveness under load.

I guess that might be a topic for another PR though. Since this works and looks solid, I'll go ahead and merge. I'll open an issue on the potential simplification.

@garlick garlick merged commit 879ff60 into flux-framework:master Jan 15, 2018

5 checks passed

buildbot/core_standard Build done.
Details
codecov/patch 78.43% of diff hit (target 78.29%)
Details
codecov/project Absolute coverage decreased by -0.07% but relative coverage increased by +0.13% compared to 24e13de
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.04%) to 78.547%
Details

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

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.