libflux/future: minor interface changes to support composite futures #1188
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #1188 +/- ##
==========================================
- Coverage 77.78% 77.77% -0.01%
==========================================
Files 158 158
Lines 29272 29284 +12
==========================================
+ Hits 22770 22777 +7
- Misses 6502 6507 +5
|
garlick
force-pushed
the
compose_future
branch
from
6b46603 to
0b3aa3a
Compare
Problem: a composite future's init function needs to set up continuations for contained futures using the reactor it is passed, but the reactor for the contained futures was pre-set when they were created. Drop reactor argument from flux_future_create(), and add flux_future_set_reactor(). Also, if a flux_t handle is set with flux_future_set_flux(), set the set the reactor to the one associated with the flux_t handle as a side-effect. For symmetry add flux_future_get_reactor() that behaves like flux_future_get_flux(). A composite future's init function must call flux_future_get_flux () on the composite future, and flux_future set_flux() on the contained futures. Or if there is no flux_t handle involved, call flux_future_set_reactor() on the contained future with reactor passed to the init function. If neither is called on a future, flux_future_then() fails with EINVAL. Update rpc.c and future unit test.
Drop the reactor argument from the future's initialization callback. It should be accessed in the same manner as the flux_t handle now that we haver reactor get/set functions. This makes the API more uniform, and the reactor is often not used if the future contains message handlers instead of reactor watchers. Update rpc implementation and unit test.
Add a test for a future containing a reactor watcher, and then a future containing two of those futures.
Add a test for a future containing an arbitrary number of futures that are created on the fly from the contained futures' continuations until the job is done.
|
Ok, discussed with @garlick offline in a small bit of depth and this is definitely reasonable cleanup at this point. Merging.. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noted when implementing some (forthcoming) KVS work that used composite futures (futures within futures, e.g. futures that perform multiple RPCs) that the internal future interfaces were not quite flexible enough to support composite futures, and that we had no tests for composite futures.
This PR makes a minor change to those interfaces, dropping the reactor argument from
flux_future_create()andflux_future_init_f, and adding reactor get/set functions.Tests for several composite future use cases are added and man pages adjusted.