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: Refactor commit & fence - part 2 #1347

merged 11 commits into from Feb 28, 2018


4 participants
Copy link

chu11 commented Feb 28, 2018

Following up #1343, this de-couples portions of the "commit manager" and creates a new "fence manager". As the internal commit API and internal fence API were decoupled in #1343, additional portions could be spliced off.

chu11 added some commits Feb 24, 2018

modules/kvs: Refactor internal commit API function
Refactor commit_mgr_process_fence_request() to take a fence_t
pointer instead of a fence name.

This patch effectively reverts

commit e2e038b
Author: Albert Chu <>
Date:   Thu Jan 4 17:14:01 2018 -0800

    modules/kvs: Refactor commit API function

as it is necessary to decouple fence storage out of the internal
commit API.
modules/kvs: Add fence manager
With recent refactoring, fences are far more decoupled from commits
than in the past.  So many aspects of "fences" in the internal
commit API can be spliced out.

So add a fence manager to the internal fence API.  The fence manager
will manage current fences active within the kvs, features that were
once managed by the commit manager.

The new fence manager largely handles responsibilities once handled
by the functions commit_mgr_add_fence(), commit_mgr_lookup_fence(),
commit_mgr_iter_not_ready_fences(), commit_mgr_remove_fence(),
and commit_mgr_fences_count().
modules/kvs/fence: Refactor fence iteration
Refactor fence_mgr_iter_not_ready_fences() to fence_mgr_iter_fences(),
where fences are not checked for "readiness".  Now that fence management
has been refactored out of the internal commit API, the concept of a
fence being "ready" or "not ready" doesn't make sense.
modules/kvs: Add fence convenience functions
Support new flux_get_processed() and flux_set_processed() convenience

Add unit tests appropriately.
modules/kvs: Refactor flagging of fences as ready
Refactor how fences are flagged as "ready" in the internal
commit API.  Instead of using a bitmask, get/set the "processed"
flag in fence_t structures.
modules/kvs: Refactor fence management in kvs core
Refactor kvs core file to manage fences via the fence manager
api instead of the commit manager api.
modules/kvs: Remove unused functions
Remove commit_mgr_add_fence(), commit_mgr_lookup_fence(),
commit_mgr_iter_not_ready_fences(), commit_mgr_remove_fence(),
and commit_mgr_fences_count(), as they are no longer used.

Remove now unused data structures & unit tests appropriately.

This comment has been minimized.

Copy link

coveralls commented Feb 28, 2018

Coverage Status

Coverage decreased (-0.06%) to 78.839% when pulling d8673c6 on chu11:issue1344-part1 into f2074ee on flux-framework:master.


This comment has been minimized.

Copy link

codecov-io commented Feb 28, 2018

Codecov Report

Merging #1347 into master will decrease coverage by 0.06%.
The diff coverage is 78.88%.

@@            Coverage Diff             @@
##           master    #1347      +/-   ##
- Coverage   78.58%   78.52%   -0.07%     
  Files         162      162              
  Lines       29610    29618       +8     
- Hits        23270    23258      -12     
- Misses       6340     6360      +20
Impacted Files Coverage Δ
src/modules/kvs/commit.c 78.02% <100%> (-1.05%) ⬇️
src/modules/kvs/kvsroot.c 68.31% <60%> (-0.44%) ⬇️
src/modules/kvs/kvs.c 65.45% <65.21%> (ø) ⬆️
src/modules/kvs/fence.c 88.59% <84.74%> (-4.74%) ⬇️
src/common/libflux/keepalive.c 86.66% <0%> (-6.67%) ⬇️
src/common/libflux/ev_flux.c 97.56% <0%> (-2.44%) ⬇️
src/common/libflux/handle.c 83.66% <0%> (-2.23%) ⬇️
src/common/libflux/rpc.c 93.38% <0%> (-0.83%) ⬇️
src/common/libflux/response.c 83.73% <0%> (-0.82%) ⬇️
src/connectors/local/local.c 87.4% <0%> (-0.75%) ⬇️
... and 5 more

This comment has been minimized.

Copy link

garlick commented Feb 28, 2018

This seems fine to me - ready for merge?

I'm hoping #1344 happens soon so we can get the terminology on track, and minimize commit messages that explain changes using the "wrong" terms.


This comment has been minimized.

Copy link
Contributor Author

chu11 commented Feb 28, 2018

yup, its ready. Next PR will rename atleast some of the code to the right terminology in the kvs module (debating order of doing things).

@garlick garlick merged commit 5192096 into flux-framework:master Feb 28, 2018

4 checks passed

codecov/patch 78.88% of diff hit (target 78.58%)
codecov/project Absolute coverage decreased by -0.06% but relative coverage increased by +0.3% compared to f2074ee
continuous-integration/travis-ci/pr The Travis CI build passed
coverage/coveralls Coverage decreased (-0.06%) to 78.839%

@chu11 chu11 self-assigned this Mar 1, 2018

@chu11 chu11 added this to To do in multi-user parallel jobs via automation Mar 1, 2018

@chu11 chu11 moved this from To do to Done in multi-user parallel jobs Mar 5, 2018

@grondo grondo referenced this pull request May 10, 2018


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.