9:11 a.m.
On 2/7/13 2:55 PM, Pedro Ruivo wrote:
> The default would be to invoke the old handle(Message) method. The
> dispatching mechanism could be changed to use the async method by
> setting an attribute in MessageDispatcher (which in turn sets it in
> RequestCorrelator).
>
> How would you do this ? Remember, we cannot change or remove
> handle(Message) as subclasses of RpcDispatcher or MessageDispatcher, or
> impls of RequestHandler are out there and any change to handle(Message)
> would break them.
>
> Would you simply provide a separate AsyncRequestHandler interface, not
> extending RequestHandler ? This would require RequestCorrelator and
> MessageDispatcher to have 2 refs instead of 1. With the current approach
> I can do an instanceof on the RequestHandler.
>
> I eventually like to merge RequestHandler and AsyncRequestHandler into 1
> class, but this can be done in 4.0 at the earliest time.
>
My opinion, I would use a separate a interface and 2 references (while
RequestHandler is still active).
That's not good because that would be an API change and would break code
in MuxRpcDispatcher/MuxRequestCorrelator.
I've pushed my branch JGRP-1587 to the repo, so you can look at the
code. This isn't final yet of course.
However, I will change the
AsyncRequestHandler interface to:
Object handle(Message message, Response response) throws Exception
OK, that's what it is currently
and the Response interface would be as follow:
void sendResponse(Object reply, boolean isException) //or sendReply(...);
Yes
void setAsyncResponse(boolean value); //or setAsyncReply(...);
boolean isAsyncResponse(); //or isAsyncReply();
I don't like this. Intuitively, we create a response or not, depending
on information in the request. Also, 2 more method to implement.
And with this, it can supports both behaviors, with a minor addition:
1) To work in async mode:
the application invokes setAsyncResponse(true) and eventually it
will invoke the sendResponse(...)
2) To work in sync mode:
the application invokes setAsyncResponse(false). I think this
should be the default value
in the RequestCorrelator, it checks if isAsyncReponse() value. If true,
it does not send the response (it only sends it if it is an exception
caught). If the value is false, it sends the return value as a response
(the current behavior). In pseudo code:
try {
ret = asyncRequestHandler.handle(message, response);
if (response expected and !response.isAsynResponse())
reponse.sendResponse(ret, false);
catch Throwable t
reponse.sendResponse(t, true)
With this suggestion, when you remove the RequestHandler interface (in
4.0 or 5.0), the user only needs to add the Response parameter and
everything will work as usual.
For Infinispan 5.3, it can be adopted as soon as it is implemented in
JGroups without using the async method.
To avoid the problem of sending twice a response, I would put an
AtomicBoolean or a similar synchronization mechanism in the Response
implementation.
opinions?
//more below
>>> - AsyncRequestHandler will look similar to the following:
>>> void handle(Message request, Handback hb, boolean requires_response)
>>> throws Throwable;
>>> - Handback is an interface, and its impl contains header information
>>> (e.g. request ID)
>>> - Handback has a sendReply(Object reply, boolean is_exception) method
>>> which sends a response (or exception) back to the caller
>>> - When requires_response is false, the AsyncRequestHandler doesn't need
>>> to invoke sendReply()
>> My 2 cents, I think that the boolean requires_response should be in
>> Handback implementation to avoid passing an extra argument
> I actually removed the requires_response parameter. If no response is
> required, rsp in handle(Message req, Response rsp) will simply be null.
I would avoid put the rsp in null and I would do an empty implementation
of the Response interface, in which the methods do nothing.
Can't do that as I want to do stuff like:
void handle(Message req, response rsp) {
try {
// invoke the request
}
catch(Throwable t) {
if(rsp != null)
rsp.send(t, true); // send the exception back to the caller
else
logError();
}
}
--
Bela Ban, JGroups lead (http://www.jgroups.org)