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 with internal kvsroot file #1321

Merged
merged 10 commits into from Jan 25, 2018

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented Jan 24, 2018

This branch isn't ready for merging (need to add documentation & unit tests) but thought I'd push to ask for comments.

I had originally gone down the path of abstracting the kvsroot way more than what is done in this PR. But as I was refactoring, it became clear abstracting struct kvsroot was creating a complex and probably unnecessary level of abstraction.

For example, abstracting start_root_remove() in kvs.c. It's a good idea to want to abstract it away, but its difficult to abstract away the cleanup of fences sitting in the commit manager. Do we ask the user to pass in a callback function to deal with the cleanup of fences? We aren't abstracting very well if we're asking the user for that.

I briefly considered moving everything related to commits into kvsroot.c, but seemed like too much. So I think other refactoring needs to be done before moving onto refactoring kvsroot more. Some thoughts for the future.

  • rethink how fences are cleaned up in its entirety (i.e. lookup stored msg copies based on name maybe not best). Perhaps some callback mechanism would work (i.e. when fence deleted, callback something to complete it.)
  • rethink entire abstraction of "fences" in fence.[ch]. Perhaps everything should be collapsed into the commit manager in some way.
modules/kvs: Fix corner case
commit_mgr_iter_not_ready_fences() should clean up the remove list
on a callback error.
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jan 24, 2018

Looks like a good direction.

I restarted a travis build that had stalled for no apparent reason.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 24, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@c2e368e). Click here to learn what that means.
The diff coverage is 71.56%.

@@           Coverage Diff            @@
##             master   #1321   +/-   ##
========================================
  Coverage          ?   78.2%           
========================================
  Files             ?     156           
  Lines             ?   28269           
  Branches          ?       0           
========================================
  Hits              ?   22107           
  Misses            ?    6162           
  Partials          ?       0
Impacted Files Coverage Δ
src/modules/kvs/commit.c 78.51% <66.66%> (ø)
src/modules/kvs/kvsroot.c 68.42% <68.42%> (ø)
src/modules/kvs/kvs.c 65.24% <74.52%> (ø)
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 24, 2018

Coverage Status

Coverage increased (+0.003%) to 78.53% when pulling 0f9ec32 on chu11:kvsrootrefactor into c2e368e on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 24, 2018

ok, let me add documentation & unit tests, then this should be all good to go.

chu11 added some commits Jan 17, 2018

modules/kvs: Begin kvsroot internal API
Move destroy_root(), remove_root(), create_root(), lookup_root(),
and lookup_root_safe() over to new files.

Adjust code appropriately given move.
modules/kvs: Update functions names in kvsroot
Prefix kvsroot to kvsroot internal API functions, adjusting functions names
appropriately and updating callers.
modules/kvs: Add kvsroot_iter function
Adjust core kvs callers that iterate manually through the roothash.
modules/kvs: Abstract kvsroot_mgr_t in kvsroot
Adjust/change function names for abstraction creation.
modules/kvs: Refactor kvsroot_mgr_create()
Variables previously passed to kvsroot_mgr_create_root(), now handled
by the manager itself in kvsroot_mgr_create().
modules/kvs: Abstract zhash kvsroot structure
Allow removal of entries from kvsroot roothash while iterating by
abstracting away remove logic from the user.

@chu11 chu11 force-pushed the chu11:kvsrootrefactor branch from aa57717 to 0f9ec32 Jan 25, 2018

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 25, 2018

added a few header comments in kvsroot.h where I thought they were necessary. Then added unit tests. I already squashed all changes into the original set as there weren't too many differences with the comment header changes.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Jan 25, 2018

Looks good @chu11 - no coverage report here (?) but it was +0.003% in the last report and it seems it can't help but have gone up from there with the new tests, so I'm good to merge if you're ready.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Jan 25, 2018

yup, all done.

@garlick garlick merged commit 7969ee6 into flux-framework:master Jan 25, 2018

5 checks passed

buildbot/core_standard Build done.
Details
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.003%) to 78.53%
Details

@garlick garlick referenced this pull request Feb 5, 2018

Closed

kvs refactor "kvsroot" #1317

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