Closed Happy0 closed 5 years ago
You will need to rebase this branch, sorry. I squashed the previous PR.
Let me know if you can rebase and clean up and I'll take a good pass at it. I can also do it for you time depending.
Thanks @atoulme - I've just rebased this against master :).
Feel free to give it a review.
Thanks for the review, @atoulme .
I'll rework the following and then update this PR again:
scuttlebutt-mux
package and just move the functionality into the existing scuttlebutt-rpc
module.AsyncResult
rather than CompletableFuture
:+1: whatever helps you get the job done.
@atoulme , the pull request is ready for review again.
I made the following changes:
The RPCHandler
class takes separate type arguments for async
requests and source
requests - this makes the previous awkward exceptions redundant. These classes also make it easier to build an RPC request (up until now we've been hand crafting the JSON for testing.)
Added integration tests for posting messages (async
) and reading a stream of your own messages (source
) as proof of concept.
Used AsyncResult
and associated classes rather htan raw CompletableFuture
.
Removed the scuttlebutt-mux
module and moved the classes to scuttlebutt-rpc
instead which makes more sense.
Added documentation and fixed formatting.
Realised I didn't have to change the ServerHandler
factory like i did to the ClientHandler
factory and associated methods since we're telling the Vertx handlers how to handle incoming requests and messages - not returning a Future
to be used.
Added locking to RPCHandler
to take edge cases of connection breaks into account. We could perhaps be more fine grained about the concurrency if needed.
ObjectMapper passed as a parameter to methods which serialize / deserialize Java objects to allow for customisation / configuration of the object mapper instance. For example, I am using this library from a Scala app and ObjectMapper needs to be configured to allow it to work with the build in collection classes, etc.
@atoulme - any idea when you'll get a chance to review this?
Thanks @atoulme !
There is still work to do in this pull request, but I wanted to open it early to make sure you are okay with the general direction before getting too deep into it. This depends on the commits from https://github.com/ConsenSys/cava/pull/201 , so the changes from that pull request are also in the 'files changed.'
This introduces functionality which allows you to make an RPC request and receive a future which will be completed when the server responses to the RPC message with the same request number or start a stream which will be updated on each arriving RPC message with the original stream's request number.
This will allow higher level libraries to expose RPC method calls building on these primitives. For example, we could have a
whoAmI
method with the signatureFuture<PublicKey> whoAmI()
which uses these primitives under the hood.Additionally, I modified the
ClientHandlerFactory
and associated classes inscuttlebutt-rpc
to use generics so that they can be constructed with something which extendsClientFactory
Does this general approach make sense to you?
TODO:
- [ ] Make the corresponding changes for the Server classes (at the moment this PR focuses on the client functionality as this is what I'm going to be using for my project.)