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

tweak watcher structure, add external watcher construction interface #1082

Merged
merged 4 commits into from May 31, 2017

Conversation

Projects
None yet
4 participants
@grondo
Copy link
Contributor

grondo commented May 31, 2017

This PR is not intended to be merged, but just to get feedback.

Following up on @garlick's similar changes to allow creation of custom watchers outside of reactor.c, these changes slightly modify watcher internals and expose flux_watcher_create and some accessors for the opaque watcher type.

However, I'm not sure if the approach goes too far, or not far enough, or if we should try something else.

The changes are pretty simple, however, highlights are:

  • The integer per-class signature was dropped because it didn't seem like there was a feasible way to guarantee uniqueness for new and/or custom watchers.
  • The watcher operations structure was replaced with a pointer to a global per-class flux_watcher_ops structure, since there should just be one global/static operations structure per "class" not per watcher. (If there is a good reason to copy in the ops structure to the watcher, I'm sorry I didn't know it)
  • The ops pointer can now be used to check for the class of watcher, replacing the previous use case for the watcher signatures.
  • Accessors were added to return the void *impl implementation data as well as the address of the operations structure
  • Since there's now an accessor for impl, both the return by reference argument toflux_watcher_create and the explicit void * impl argument to the watcher start,stop,destroy methods were dropped.

grondo added some commits May 23, 2017

libflux/reactor: simplify watcher creation
Add a function to return watcher implementation pointer (impl) and
use this function in watcher constructors to simplify the generic
flux_watcher_create() call.
libflux/reactor: don't copy watcher ops into each instance
Make the 'ops' vtable member of flux_watcher into a pointer to an
watcher_ops structure, instead of copying the vtable into each instance
of the watcher class.

This then obviates the need for watcher "signatures", because the
address of the operations structure can be used to uniquely identify
each "class" of watcher.

Besides, since the signatures were defined inside the source of
reactor.c, there was no way to guarauntee that any externally created
watchers would have a different signature from existing watchers;
libflux/reactor: allow externally created watchers
Allow code outside of reactor.c to create watchers, and expose
flux_watcher_create(), flux_watcher_ops, etc. as part of the Flux
API.
libflux/reactor: simplify watcher interface methods
There's no need to pass watcher implementation pointer `impl` to the
watcher interface functions. Watchers internally defined in reactor.c
can use w->impl, and externally defined watchers can use the
flux_watcher_impl () accessor.
@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 31, 2017

This looks like a good cleanup to me!

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 31, 2017

Coverage Status

Coverage decreased (-0.001%) to 78.187% when pulling e2164c7 on grondo:watchers into 17347fd on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 31, 2017

Codecov Report

Merging #1082 into master will decrease coverage by <.01%.
The diff coverage is 89.89%.

@@            Coverage Diff             @@
##           master    #1082      +/-   ##
==========================================
- Coverage   77.93%   77.93%   -0.01%     
==========================================
  Files         150      150              
  Lines       25954    25934      -20     
==========================================
- Hits        20228    20212      -16     
+ Misses       5726     5722       -4
Impacted Files Coverage Δ
src/common/libflux/reactor.c 92.89% <89.89%> (-2%) ⬇️
src/common/libflux/attr.c 90.81% <0%> (-2.05%) ⬇️
src/common/libflux/response.c 83.76% <0%> (-0.86%) ⬇️
src/broker/overlay.c 71.32% <0%> (-0.35%) ⬇️
src/common/libflux/handle.c 85.38% <0%> (-0.29%) ⬇️
src/modules/kvs/kvs.c 79.53% <0%> (+0.38%) ⬆️
src/broker/modservice.c 80.19% <0%> (+0.99%) ⬆️
src/broker/content-cache.c 74.29% <0%> (+1.3%) ⬆️
src/broker/module.c 83.66% <0%> (+1.4%) ⬆️
@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 31, 2017

Discussed this briefly wtih @grondo in the hallway, and we agreed this could be merged if tests passed.

Thanks for the cleanup!

@garlick garlick merged commit d453552 into flux-framework:master May 31, 2017

4 checks passed

codecov/patch 89.89% of diff hit (target 77.93%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +11.96% compared to 17347fd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.001%) to 78.187%
Details

@grondo grondo deleted the grondo:watchers branch May 31, 2017

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

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.