-
Notifications
You must be signed in to change notification settings - Fork 584
Add CompletableFuture async RPC to channel #257
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
Conversation
References #215
@acogoluegnes is this ready for QA? It was submitted in March but there is no "DO NOT MERGE" in the title. |
@michaelklishin Yes, it's ready for QA. |
Conflicts: src/test/java/com/rabbitmq/client/test/ClientTests.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This turned out to be more straightforward than I expected but there's a couple of small things that need addressing.
} | ||
}, executor).thenAcceptAsync(action -> { | ||
try { | ||
channel.basicPublish("", queue, null, "dummy".getBytes()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should wait for a confirm here, otherwise there can be a race condition between a message send by the channel to the queue master process and handling of the basic.consume
below.
@@ -135,6 +137,20 @@ public AMQCommand exnWrappingRpc(Method m) | |||
} | |||
} | |||
|
|||
public CompletableFuture<Command> exnWrappingAsyncRpc(Method m) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a public method, I'd like to clarify the name. How about exceptionWrappingAsyncRpc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now see we already have a "non-async" method with a similar name from way back. Disregard the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added publisher confirms to the test.
This |
@ofavre If you mean publishing a message asynchronously, Please move the discussion to rabbitmq-users if you have further questions. |
None of this code is meant to be used directly. Use the |
Add
CompletableFuture<Command> asyncCompletableRpc(Method method)
toChannel
. This is foundation to react to asynchronous AMQP methods (most of the resource management methods). Right now, the RPC calls are serialized at the channel level, but this could be improved in the future by using a queue for the outstanding RPC calls.Fixes #215