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

libflux/rpc: add flux_rpcf_multi(), add "any" nodeset option, various refactoring #909

Merged
merged 11 commits into from Nov 18, 2016

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Nov 17, 2016

Another spin-off from #898 as the patch series is long. A fair amount of refactoring, adding support for flux_rpcf_multi() the jansson equivalent to flux_rpc_multi(). Compared to my original patch series in #898, flux_rpc[f]_multi now constructs a single flux message for all rpcs instead of a message on each iteration. So the refactoring is now cleaner and it should shave off a few instructions for the old case.

An "any" special case option for the nodeset input was added to the flux_rpc[f]_multi functions for convenience. Passing "FLUX_NODEID_ANY" appears to be the one case that can't be
handled by flux_rpc[f]_multi. So tools needed a special case to deal with that and call flux_rpc(). This hopefully removes that need for that special case handling. I know it's one more string comparison in the function, but figured it was no big deal since the for loop in the middle is the big cost.

Also, fixed #905 along the way.

@garlick garlick added the review label Nov 17, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 17, 2016

Coverage Status

Coverage decreased (-48.2%) to 27.714% when pulling 2f73515 on chu11:fluxrpcfmulti into 55dcaaf on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 17, 2016

Just had a quick spin through each commit and this seems like good cleanup!

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 17, 2016

An "any" special case option for the nodeset input was added to the flux_rpc[f]_multi functions for convenience. Passing "FLUX_NODEID_ANY" appears to be the one case that can't be
handled by flux_rpc[f]_multi

I probably asked this before, but since flux_rpc{f}_multi now supports sending to FLUX_NODEID_ANY, is there any way to unify the two under a single function?

@chu11 chu11 force-pushed the chu11:fluxrpcfmulti branch from 2f73515 to f886cb5 Nov 17, 2016

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 17, 2016

Current coverage is 72.38% (diff: 100%)

Merging #909 into master will increase coverage by 0.03%

@@             master       #909   diff @@
==========================================
  Files           157        157          
  Lines         27042      27029    -13   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          19567      19566     -1   
+ Misses         7475       7463    -12   
  Partials          0          0          
Diff Coverage File Path
•••••••••• 100% src/common/libflux/rpc.c

Powered by Codecov. Last update a2de3c4...b4b330c

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 17, 2016

@grondo By unifying flux_rpc_multi and flux_rpcf_multi do you mean that any code calling the old json-c functions could degenerate into calling the jansson functions by just passing in the json string as the fmt argument?

I suppose that'd make sense, although I'm wondering about the subtlety in the jansson pack API and what formatting it may like/not-like. We'd have to consider unifying flux_rpc and flux_rpcf too.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 17, 2016

I probably asked this before, but since flux_rpc{f}_multi now supports sending to FLUX_NODEID_ANY, is there any way to unify the two under a single function?

Are you proposing unifying flux_rpc{f} and flux_rpc_multi{f}?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 17, 2016

Coverage Status

Coverage increased (+0.01%) to 75.969% when pulling f886cb5 on chu11:fluxrpcfmulti into 55dcaaf on flux-framework:master.

@chu11 chu11 force-pushed the chu11:fluxrpcfmulti branch from f886cb5 to fc4e454 Nov 17, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 17, 2016

Coverage Status

Coverage increased (+0.009%) to 75.965% when pulling fc4e454 on chu11:fluxrpcfmulti into 55dcaaf on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 17, 2016

travis finally passed, it caught a few warnings in multrpc.c that weren't caught on toss2 hype.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 17, 2016

I guess if "any" is supported, "upstream' should be also. That would let flux_rpc_multi{f} handle all the cases that flux_rpc{f} handles...

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 17, 2016

I had thought of that, but didn't see a present use case. I'll add that in for thoroughness.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 18, 2016

I had thought of that, but didn't see a present use case.

I guess it makes commands a little more flexible. Like maybe flux ping upstream!kvs or similar would just work?

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 18, 2016

that is indeed a good use case :-) I wasn't actually going to document "any" (or "upstream") as a possible option to flux-ping, but I suppose they can be advertised as options.

Is "any" something we'd want to add to all tools as an option to --rank options? Of course it's not supported in all library calls, so it'd have to be handled manually within a number of tools.

@chu11 chu11 force-pushed the chu11:fluxrpcfmulti branch from fc4e454 to cee67a2 Nov 18, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 18, 2016

Coverage Status

Coverage increased (+0.02%) to 75.974% when pulling cee67a2 on chu11:fluxrpcfmulti into 55dcaaf on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 18, 2016

I guess if "any" is supported, "upstream' should be also. That would let
flux_rpc_multi{f} handle all the cases that flux_rpc{f} handles...

Ah, sorry I had forgotten about FLUX_NODEID_UPSTREAM, but yes I was just
wondering if we do all this work to make flux_rpc_multi support all cases
of flux_rpc, why not make just make one flux_rpc API call, and avoid
callers having to think about which to use?

I actually don't feel too strongly about it, but it also seemed strange to
have a "rpc_multi" call with specific support for singleton RPC
destinations like"any" and "upstream". Unifying under a single flux_rpc
call would make that seem natural.

That being said, I can see why the "any" support makes sense for flux ping,
and I don't disagree with the changes.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 18, 2016

pushed branch w/ "upstream", so I think this one is good to go

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Nov 18, 2016

I do have some more work I'd like to do on the "multi" interface to support other multiple rpc idioms, including multiple rpcs to the same server and cascading rpcs, where internally a new rpc is generated based on the response of the last rpc. I'm not 100% sure that will work out, but I'd rather not make a user-visible API change like consolidating flux_rpc() and flux_rpc_multi() while that's still pending.

I think this is a good PR and can go in. It does need to be rebased on current master. Thanks @chu11!

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 18, 2016

@grondo Ahh, I see your point. I guess my only thought is by avoiding the string comparisons and a lot of the overhead of flux_multi_rpc(), there are many situations where the single call would be much better for performance? Presumably the extra overhead is only worthwhile for flux_multi b/c of the assumption of the loop being the big O(N) in the middle.

Perhaps we can start and issue and discuss for a future PR?

@garlick garlick referenced this pull request Nov 18, 2016

Merged

add flux_content API #903

chu11 added some commits Nov 10, 2016

libflux/rpc: Refactor rpc_request_send functions
Create new common function rpc_request_prepare_send(), utilized
by rpc_request_send(), rpc_request_vsendf(), rpc_request_send_raw().
libflux/rpc: Refactor flux_rpc_multi
Refactor flux_rpc_multi. Create new function rpc_multi that takes
a flux msg to send in an rpc.
libflux/rpc: Add flux_rpcf_multi
Add flux_rpcf_multi, the jansson pack/unpack equivalent to
flux_rpc_multi.
libflux/rpc: Add missing caliper calls
Add missing caliper calls in flux_rpc_raw()

Fixes #905
libflux/rpc: Refactor flux_rpc calls
Refactor flux_rpc(), flux_rpc_raw(), and flux_rpcf() to use
new common function rpc_request().

Remove now unnecessary functions rpc_request_send(),
rpc_request_vsendf(), and rpc_request_send_raw().
libflux/rpc: Support special case strings
Support "any" and "upstream" nodeset strings in rpc_multi().
These will send a single RPC to FLUX_NODEID_ANY or
FLUX_NODEID_UPSTREAM respectively.  This support is for convenience,
so that any code that may need to send to FLUX_NODEID_ANY/UPSTREAM
is not required to setup a special case call to flux_rpc() instead
of flux_rpc_multi().
doc/flux_rpc_multi(3): Document new options
Document that "any" and "upstream" are a valid nodeset options in
flux_rpc_multi(3).

chu11 added some commits Nov 16, 2016

test/multrpc: Add new nodeset tests
Add tests to test "any" and "upstream" nodeset
doc/flux_rpc_multi(3): Add flux_rpcf_multi()
Add documentation on flux_rpcf_multi() and add include info on
encoding with json_pack().
test/multrpc: Refactor for new tests
Create wrapper run_multi_test() for future new tests.
Create rpctest_set_size() to set test rank size.
Initialize then_count appropriately.
test/multrpc: Add flux_rpcf_multi() tests
As consequence, use flux_respondf() and flux_rpc_getf() in tests
as well.

@chu11 chu11 force-pushed the chu11:fluxrpcfmulti branch from cee67a2 to b4b330c Nov 18, 2016

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Nov 18, 2016

@garlick just rebased, going through travis now

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 18, 2016

Coverage Status

Coverage increased (+0.03%) to 75.996% when pulling b4b330c on chu11:fluxrpcfmulti into a2de3c4 on flux-framework:master.

@garlick garlick merged commit c5478da into flux-framework:master Nov 18, 2016

4 checks passed

codecov/patch 100% of diff hit (target 72.35%)
Details
codecov/project 72.38% (+0.03%) compared to a2de3c4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 75.996%
Details

@garlick garlick removed the review label Nov 18, 2016

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Nov 18, 2016

I guess my only thought is by avoiding the string comparisons and a lot of the overhead of flux_multi_rpc(), there are many situations where the single call would be much better for performance?

Yes, good point. Like I said I was just bringing up the point because I think it will be other users' first thought when they look at these interfaces. I'm fine with things as they are.

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.