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

broker/service: cleanup and prepare for dynamic service registration #1189

Merged
merged 10 commits into from Sep 14, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Sep 13, 2017

This PR was prep work for dynamic service registration, but I switched to other priorities before getting too far into it a month ago. What was completed is valid cleanup, so I thought it preferable to submit as a cleanup PR than let it languish until I get back to the original topic.

Some exit-on-ENOMEM behavior was avoided, some style violations were set right, and some poor naming was (hopefully) improved. The broker "service hash" was prepped to allow for an arbitrary association of service names to callbacks (which refer to broker services or modules), instead of the hardwired primary name and secondary alias. And a test was added for that class.

garlick added some commits Aug 14, 2017

broker/service: [cleanup] drop svchash_t typedef
Drop svchash_t typedef, which is not helpful.

Rename struct svchash_struct -> struct service_switch per RFC 7.

Rename svchash_create() -> service_switch_create().
Rename svchash_destroy() -> service_switch_destroy().
broker/service: [cleanup] drop svc_t typedef
Drop svc_t typedef, which is not helpful.

Rename struct svc_struct -> struct service per RFC 7.

Make struct service private to service.c, changing the
return value of svc_add() from svc_t * to an integer
(0 on success, -1 on failure), and update broker users.

Fix up one place in the broker where svc_add() errno was
overwritten with EEXIST.
broker/service: [cleanup] rename service_send_f
Rename svc_cb_f -> service_send_f, to be more descriptive.
broker/service: [cleanup] rename service_add,remove
Rename svc_add() -> service_add(),
and svc_remove() -> service_remove()
for uniform naming within "service" class.
broker/service: [cleanup] rename service_send
Rename svc_sendmsg() to service_send() for uniform
naming with in "service" class.
broker/service: [cleanup] rename internal functions
Rename internal functions
svc_create() -> service_create()
svc_destroy() -> service_destroy()
for uniform naming within the "service" class
(even though these functions are not currently public).
broker/service: allow multiple service names
Instead of only allowing one service name with an optional alias,
allow any number of services to be registered with the same callback.
Pass in a uuid with each registration and add a function to remove
all services registered by a given uuid.

Modify the broker so that modules register their primary and
alias service names with the module uuid, and remove all services
by uuid when the module unloads.

Optimize service_send() a bit so that a malloc is not incurred
each time we want to lookup a service by a substring of a message's
topic string.  (If it's short, use a stack variable).

Fix up remaining spots where exit is called on malloc failure.
broker: only register single-word service names
Problem: we inconsistently register service names for
individual request handlers in the broker, with the same
callback.  This complicates the service lookup code
(since it has to try to lookup multiple substrings of inbound
message topic strings).  It's also unnecessary.

Clean this up so we only register single word services.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 13, 2017

Coverage Status

Coverage decreased (-0.02%) to 78.137% when pulling 9991071 on garlick:dyn_service into 8443eff on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 13, 2017

Restarted two travis builders - one hit a write error and the other timed out.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 13, 2017

Codecov Report

Merging #1189 into master will decrease coverage by 0.03%.
The diff coverage is 84.52%.

@@            Coverage Diff             @@
##           master    #1189      +/-   ##
==========================================
- Coverage   77.82%   77.78%   -0.04%     
==========================================
  Files         158      158              
  Lines       29257    29271      +14     
==========================================
  Hits        22768    22768              
- Misses       6489     6503      +14
Impacted Files Coverage Δ
src/broker/broker.c 72.98% <81.25%> (-0.15%) ⬇️
src/broker/service.c 86.84% <85.29%> (-11.44%) ⬇️
src/common/libflux/ev_flux.c 97.56% <0%> (-2.44%) ⬇️
src/common/libkvs/kvs_watch.c 86.34% <0%> (-0.89%) ⬇️
src/common/libflux/msg_handler.c 84.81% <0%> (-0.75%) ⬇️
src/connectors/local/local.c 87.4% <0%> (-0.75%) ⬇️
src/common/libflux/reactor.c 93.18% <0%> (-0.29%) ⬇️
src/modules/kvs/kvs.c 63.16% <0%> (-0.27%) ⬇️
src/common/libkvs/kvs.c 65.08% <0%> (-0.26%) ⬇️
src/modules/connector-local/local.c 74.27% <0%> (-0.19%) ⬇️
... and 7 more
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 13, 2017

This looks like nice cleanup to me! Out of plain curiosity, why s/svchash/service_switch/ and not service_hash or similar (more curious just because I think I could learn something).

I didn't look too close at the tests, but was a testcase added for a service registered under multiple names? Sorry if that was already in there just wanted to doublecheck.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 13, 2017

You are right, I didn't cover that. Will add.

On the name, well... it's sort of a switchyard for messages - message goes in, destination is looked up based on topic string, destination's callback made to send message on its way. Maybe silly but it kind of worked for me.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 14, 2017

Coverage Status

Coverage decreased (-0.03%) to 78.126% when pulling c004490 on garlick:dyn_service into 8443eff on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 14, 2017

This looks good to me. Ready for merge?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Sep 14, 2017

Sure.

@grondo grondo merged commit 4157888 into flux-framework:master Sep 14, 2017

4 checks passed

codecov/patch 84.52% of diff hit (target 77.82%)
Details
codecov/project Absolute coverage decreased by -0.03% but relative coverage increased by +6.7% compared to 8443eff
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 78.126%
Details

@garlick garlick deleted the garlick:dyn_service branch Sep 14, 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.