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

pmi: async kvs_get support for simple PMI server #1615

Merged
merged 1 commit into from Jul 30, 2018

Conversation

Projects
None yet
4 participants
@grondo
Copy link
Contributor

grondo commented Jul 30, 2018

As discussed in #1606, split the simple PMI kvs_get operation into kvs_get and kvs_get_complete calls so that an implementation may utilize asynchronous calls when available.

Switch all call sites to the new kvs_get prototype, and use async version in wrexexcd's implementation to (hopefully) mitigate serialization when wrexecd is managing many local tasks.

Fixes #1609

pmi: async kvs_get support for simple PMI server
Problem: The simple PMI server's `kvs_get` operation doesn't allow
the underlying implementation to use asynchronous flux_kvs_lookup()
calls. Since all local PMI tasks connect to the same server, this
serializes kvs lookups in the PMI server implementation, and may
impact scalability of PMI launch with many local tasks.

Split the simple PMI kvs_get operation into  `kvs_get` and
`kvs_get_complete` calls so that an implementation may utilize
asynchronous calls when available.

Switch all call sites to the new kvs_get prototype, and use async
version in wrexexcd's implementation to (hopefully) mitigate
serialization when wrexecd is managing many local tasks.

Fixes #1609
@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Jul 30, 2018

looks pretty straight forward. ready to merge?

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 30, 2018

Codecov Report

Merging #1615 into master will decrease coverage by 0.01%.
The diff coverage is 73.52%.

@@            Coverage Diff             @@
##           master    #1615      +/-   ##
==========================================
- Coverage   79.36%   79.35%   -0.02%     
==========================================
  Files         171      171              
  Lines       31461    31473      +12     
==========================================
+ Hits        24970    24975       +5     
- Misses       6491     6498       +7
Impacted Files Coverage Δ
src/common/libpmi/simple_server.c 52% <100%> (+2.6%) ⬆️
src/modules/wreck/wrexecd.c 76.06% <57.89%> (+0.19%) ⬆️
src/cmd/flux-start.c 80.71% <66.66%> (-0.29%) ⬇️
src/broker/module.c 83.79% <0%> (-1.4%) ⬇️
src/broker/content-cache.c 72.88% <0%> (-1.28%) ⬇️
src/common/libflux/message.c 81.02% <0%> (+0.11%) ⬆️
src/common/libflux/handle.c 83.91% <0%> (+0.24%) ⬆️
src/common/libflux/future.c 86.2% <0%> (+0.31%) ⬆️
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 30, 2018

Coverage Status

Coverage decreased (-0.01%) to 79.52% when pulling 3ff31ca on grondo:pmi-async-kvs into 1580764 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Jul 30, 2018

Yeah, sadly the coverage is low due to high proportion of error handling code, but I think since this patch was tested already at medium scale it is low risk to go in now.

Thanks!

@chu11 chu11 merged commit 9095ed9 into flux-framework:master Jul 30, 2018

2 of 4 checks passed

codecov/patch 73.52% of diff hit (target 79.36%)
Details
codecov/project 79.35% (-0.02%) compared to 1580764
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 79.52%
Details

@grondo grondo deleted the grondo:pmi-async-kvs branch Feb 8, 2019

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.