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: general refactoring and cleanup #1314

Merged
merged 14 commits into from Jan 9, 2018
Merged

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Jan 8, 2018

This PR has a few tiny corner case fixes, some general refactoring/abstraction fixes in the internal commit API, and some code re-organization in the core KVS file.

Callers never pass a non-NULL initial rootdir, so remove code
surrounding this option.
Check if a namespace remove is already in progress before removing
a namespace root.
Move start_root_remove() and root_remove_process_fences() closer
to the functions that use them.
Refactor commit_mgr_process_fence_request() to take a fence
name instead of a fence pointer.  API should not assume user
passed in a fence that has already been added to the commit
manager via commit_mgr_add_fence().
Using new fence aux int, set flag indicating if a fence
has already been placed on the ready list.

If it has already been placed on the ready list, don't add it to
the ready list again.
Rename to commit_mgr_iter_not_ready_fences().  We want to abstract
what "ready" vs "not-ready" fence means in this API and not rely
on the user to know.

Adjust core code in main KVS file appropriately.

Also add some extra comments on usage.
Remove struct finalize_data, is unnecessary, just use struct kvs_cb_data.
Internally manage/allow commit_mgr_remove_fence() to be called if
you are iterating through fences via commit_mgr_iter_not_ready_fences().

This abstracts commit mgr more from the user, not requiring the user
to understand internals of how the commit mgr is implemented.
Check that we are not currently iterating through fences, we
do not want to modify the hash while iterating.
Check that the fence only has one name and has not yet been
merged with another fence.
There is no need to complete the setroot if the namespace is in
process of being removed.
Recent changes to the core KVS file lead to a change in how
many of the functions in the file were called and/or used.  Those
functions now seem "out of place" given the current code.  Move
functions around and re-organize in general.

No code has changed in this commit.  Functions have only been
moved around.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 44dc055 on chu11:kvscleanup9 into ** on flux-framework:master**.

@chu11
Copy link
Member Author

chu11 commented Jan 8, 2018

hit #1150, restarting a builder

@codecov-io
Copy link

codecov-io commented Jan 8, 2018

Codecov Report

Merging #1314 into master will increase coverage by 0.05%.
The diff coverage is 64.89%.

@@            Coverage Diff             @@
##           master    #1314      +/-   ##
==========================================
+ Coverage   78.23%   78.29%   +0.05%     
==========================================
  Files         154      154              
  Lines       28100    28133      +33     
==========================================
+ Hits        21984    22026      +42     
+ Misses       6116     6107       -9
Impacted Files Coverage Δ
src/modules/kvs/fence.c 84.26% <100%> (+0.93%) ⬆️
src/modules/kvs/kvs.c 65.53% <63.29%> (+0.02%) ⬆️
src/modules/kvs/commit.c 78.67% <83.33%> (+0.15%) ⬆️
src/common/libflux/request.c 88.46% <0%> (-1.29%) ⬇️
src/broker/modservice.c 80.58% <0%> (-0.98%) ⬇️
src/common/libflux/response.c 83.73% <0%> (-0.82%) ⬇️
src/common/libflux/handle.c 83.66% <0%> (-0.25%) ⬇️
src/common/libflux/message.c 81.72% <0%> (+0.11%) ⬆️
src/common/libkvs/kvs_watch.c 88.54% <0%> (+0.88%) ⬆️
src/common/libflux/mrpc.c 86.66% <0%> (+1.17%) ⬆️
... and 2 more

@chu11
Copy link
Member Author

chu11 commented Jan 8, 2018

code coverage diff is not too good. I re-organized so much code that effectively many of the uncoverable error paths that were not-covered before are not-covered again.

The diff coverage did make me notice one other thing that could be cleaned up. The now variable is no longer used in content_store_request_send(), so we could axe that.

Refactor out "now" parameter, as it is only set to false.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 24e13de on chu11:kvscleanup9 into ** on flux-framework:master**.

@garlick
Copy link
Member

garlick commented Jan 9, 2018

If there's just the one "ready" flag, maybe a mask of flags should be replaced with set_ready(), get_ready()? Or were you anticipating more flags here?

@garlick
Copy link
Member

garlick commented Jan 9, 2018

LGTM otherwise.

@chu11
Copy link
Member Author

chu11 commented Jan 9, 2018

If there's just the one "ready" flag, maybe a mask of flags should be replaced with set_ready(), get_ready()? Or were you anticipating more flags here?

Do you mean a fence_get_ready() and/or fence_set_ready()? I decided not to do that, since the fence doesn't really have any concept of itself being "ready" or not. It's an abstracted concept that is only really known to the commit manager. Thus decided to manage the fence with an "aux integer" instead.

@garlick
Copy link
Member

garlick commented Jan 9, 2018

OK, well not a big deal to be sure.

LMK when you're done poking at it and I'll have one more spin through before merging.

@chu11
Copy link
Member Author

chu11 commented Jan 9, 2018

I think I'm done with this PR.

@garlick
Copy link
Member

garlick commented Jan 9, 2018

OK, great! Thanks.

@garlick garlick merged commit a4d438a into flux-framework:master Jan 9, 2018
@grondo grondo mentioned this pull request May 10, 2018
@chu11 chu11 deleted the kvscleanup9 branch June 5, 2021 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants