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

modules/kvs: handle root dir object caching more precisely #1177

Merged
merged 4 commits into from Sep 1, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Aug 31, 2017

This PR fixes needless alarming error messages noted in #1174, and eliminates the last use of the (internal) KVS load() function with wait == NULL. That allows load() to be simplified a bit.

garlick added some commits Aug 31, 2017

modules/kvs: don't complain of invalid root cache entry
Problem: the heartbeat event handler tries to "touch"
the root cache entry to prevent it from being temporally
expired, for performance.  It calls load() on the root
blobref, but load() complains if there happens to be
an invalid (content load pending) cache entry for the root.
Severe sounding errors are emitted:

  kvs.err[2]: synchronous load(), invalid cache entry
  kvs.err[2]: heartbeat_cb: load: No data available

Call cache_lookup() on the root blobref instead, and don't
complain if the lookup fails.

Fixes #1174
modules/kvs: [cleanup] setroot_event_send() use cache
Problem: setroot_event_send() calls load() on the root cache
entry, to look up the root dir object for inclusion in the
setroot event payload.  However, most of the code in load()
is not used here since the the root dir object is always in
cache on rank 0.

Simplify the code by calling cache_lookup() instead.
modules/kvs: [cleanup] simplify load()
The load() function includes code to respond synchronously
if wait == NULL.  There are no longer any callers that require
this, so assert that wait != NULL and eliminate dead code.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 31, 2017

Coverage Status

Coverage increased (+0.02%) to 78.119% when pulling e0bba82 on garlick:kvs_root_cache_entry into a3ebd15 on flux-framework:master.

modules/kvs: [cleanup] use flux_content_*
Use flux_content_load() et al instead of hand-crafting
content RPC's directly.
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Aug 31, 2017

restarted one test due to write error.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 31, 2017

Codecov Report

Merging #1177 into master will increase coverage by 0.09%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##           master    #1177      +/-   ##
==========================================
+ Coverage   77.73%   77.82%   +0.09%     
==========================================
  Files         158      158              
  Lines       29265    29210      -55     
==========================================
- Hits        22749    22733      -16     
+ Misses       6516     6477      -39
Impacted Files Coverage Δ
src/modules/kvs/kvs.c 63.61% <63.63%> (+0.88%) ⬆️
src/broker/modservice.c 79.61% <0%> (-0.98%) ⬇️
src/common/libflux/future.c 88.61% <0%> (-0.5%) ⬇️
src/common/libcompat/handle.c 84.97% <0%> (+0.03%) ⬆️
src/common/libflux/handle.c 83.91% <0%> (+0.24%) ⬆️
src/common/libkvs/kvs_watch.c 87.22% <0%> (+0.44%) ⬆️
src/common/libutil/dirwalk.c 92.95% <0%> (+0.7%) ⬆️
src/common/libkvs/kvs.c 65.54% <0%> (+0.91%) ⬆️
src/common/libflux/mrpc.c 86.32% <0%> (+1.17%) ⬆️
... and 1 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 31, 2017

Coverage Status

Coverage increased (+0.06%) to 78.159% when pulling 6efe022 on garlick:kvs_root_cache_entry into a3ebd15 on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Aug 31, 2017

restarted one test due to hydra test failure (documented in #1169)

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Aug 31, 2017

just skimed through the patches, looks good to me. I'll press the button once tests pass.

I'm glad we were able to figure out the "race", as it bothered me how #1174 could even happen.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 1, 2017

write error, restarted

@chu11 chu11 merged commit 59607e7 into flux-framework:master Sep 1, 2017

3 of 4 checks passed

codecov/patch 63.63% of diff hit (target 77.73%)
Details
codecov/project 77.82% (+0.09%) compared to a3ebd15
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.06%) to 78.159%
Details

@garlick garlick deleted the garlick:kvs_root_cache_entry branch Sep 1, 2017

@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.