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

reimplement RPC in terms of futures #1089

Merged
merged 12 commits into from Jun 13, 2017

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Jun 13, 2017

This is work pulled out of pr #1083 and reworked.

As discussed in #1088, the RPC interface changes so that flux_rpc() and variants returns a future, and flux_rpc_get() and variants accept a future, and all the other functions go away.

Documentation updated accordingly.

garlick added some commits Jun 11, 2017

test/rpc: avoid flux_rpc_check() busy-wait
Stop using idiom in tests where we call flux_rpc_check()
in a tight loop until a result is ready.  We have changed
flux_rpc_wait_for() so that a wait with timeout=0 does not
invoke the reactor, so the idiom won't work.  And if it did,
it would be something to discourage due to its inefficiency.
test/rolemask: avoid flux_rpc_check() busy-wait
Stop using idiom in tests where we call flux_rpc_check()
in a tight loop until a result is ready.  We have changed
flux_rpc_wait_for() so that a wait with timeout=0 does not
invoke the reactor, so the idiom won't work.  And if it did,
it would be something to discourage due to its inefficiency.
libflux/response: flux_response_decode allow raw payload
Problem: we need a way to parse a response and obtain any
embedded error without making assumptions about the payload
structure.

Change flux_response_decode() so that it only attempts to
parse a JSON payload if passed a non-NULL json_str argument.
modules/connector-local: set rolemask on responses
Responses originating from the connector-local module should
have FLUX_ROLE_OWNER.

When responses are handled via the reactor/dispatcher, the
built-in policy rejects responses with no assigned roles.
cmd/flux-proxy: set rolemask on responses
Responses originating from the flux-proxy process should
have FLUX_ROLE_OWNER.

When responses are handled via the reactor/dispatcher, the
built-in policy rejects responses with no assigned roles.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 13, 2017

Codecov Report

Merging #1089 into master will decrease coverage by <.01%.
The diff coverage is 91.76%.

@@            Coverage Diff             @@
##           master    #1089      +/-   ##
==========================================
- Coverage   77.99%   77.99%   -0.01%     
==========================================
  Files         151      151              
  Lines       26172    26105      -67     
==========================================
- Hits        20414    20361      -53     
+ Misses       5758     5744      -14
Impacted Files Coverage Δ
src/common/libflux/future.c 87.7% <ø> (+8.02%) ⬆️
src/common/libflux/panic.c 0% <0%> (ø) ⬆️
src/broker/broker.c 72.78% <0%> (ø) ⬆️
src/common/libcompat/rpc.c 100% <100%> (ø) ⬆️
src/common/libflux/module.c 76.92% <100%> (-0.91%) ⬇️
src/modules/wreck/job.c 71.37% <100%> (ø) ⬆️
src/common/libflux/flog.c 92.92% <100%> (-0.07%) ⬇️
src/cmd/flux-module.c 84.49% <100%> (+0.04%) ⬆️
src/cmd/builtin/proxy.c 72.87% <100%> (+0.05%) ⬆️
src/broker/log.c 68.32% <100%> (ø) ⬆️
... and 32 more
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 13, 2017

Coverage Status

Coverage decreased (-0.06%) to 78.192% when pulling ba1d9fc on garlick:rpc_futures2 into 89dafb9 on flux-framework:master.

garlick added some commits Jun 9, 2017

libflux/rpc: reimplement with futures
Change the RPC API to return flux_future_t instead of
flux_rpc_t.

Leave compatability in place by making flux_rpc_t an alias
for flux_future_t and aux_get/set, rpc_then, and rpc_check
there as wrappers for similar future calls.

When handling the response, call flux_response_decode()
on it and fulfill the future with an error if there was one.
This allows no-payload responses to be checked with
flux_future_get().

Fixes #1088
convert in-tree users to future based RPCs
Switch over all the in-tree users to the future based
RPC interface.
libflux/rpc: drop compat interfaces
Now that in-tree users have been converted, drop
compatabilty interfaces.
doc/flux_rpc(3): rework man page and examples
Consolidate RPC functions in one page flux_rpc(3),
adding references to flux_future_then(3) etc for
synchronization portion.

Rework example code to be more clear.

@garlick garlick force-pushed the garlick:rpc_futures2 branch from ba1d9fc to 983b17b Jun 13, 2017

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jun 13, 2017

Re-pushed with some minor man page and commit message tweaks.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 13, 2017

Coverage Status

Coverage decreased (-0.0008%) to 78.256% when pulling 983b17b on garlick:rpc_futures2 into 89dafb9 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Jun 13, 2017

I tested this branch both on my desktop and a toss 2 system and things look good.

The valgrind test is failing on hype2, but it also fails on master so I'll open a new issue on that (may be false positives)

Ready to merge now that sched PR is open?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Jun 13, 2017

Yes, thanks for the extra testing!

@grondo grondo merged commit 609d88f into flux-framework:master Jun 13, 2017

4 checks passed

codecov/patch 91.76% of diff hit (target 77.99%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +13.76% compared to 89dafb9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.0008%) to 78.256%
Details

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

@garlick garlick deleted the garlick:rpc_futures2 branch Sep 6, 2017

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.