[infinispan-dev] Threadpools in a large cluster

[Pedro Ruivo]

Hi, see inline

Cheers,
Pedro

On 2/7/13 11:42 AM, Bela Ban wrote:
> On 2/7/13 12:29 PM, Pedro Ruivo wrote:
>> Hi Bela
>>
>> On 2/7/13 4:53 AM, Bela Ban wrote:
>>> Hi Pedro,
>>>
>>> this is almost exactly what I wanted to implement !
>>>
>>> Question:
>>> - In RequestCorrelator.handleRequest():
>>>
>>> protected void handleRequest(Message req, Header hdr) {
>>> Object retval;
>>> boolean threwException = false;
>>> MessageRequest messageRequest = new MessageRequestImpl(req, hdr);
>>> try {
>>> retval=request_handler.handle(messageRequest);
>>> } catch(Throwable t) {
>>> retval=t;
>>> threwException = true;
>>> }
>>> messageRequest.sendReply(retval, threwException);//<-- should be moved
>>> up, or called only if threwException == true
>>> }
>>>
>>>
>>> , you create a MessageRequestImpl and pass it to the RequestHandler. The
>>> request handler then dispatches the request (possibly) to a thread pool
>>> and calls MessageRequestImpl.sendReply() when done.
>>>
>>> However, you also call MessageRequest.sendReply() before returning from
>>> handleRequest(). I think this is an error, and
>>> MessageRequest.sendReply() should be moved up inside the catch clause,
>>> or be called only if threwException is true, so that we send a reply on
>>> behalf of the RequestHandler if and only if it threw an exception (e.g.
>>> before it dispatches the request to a thread pool). Otherwise, we'd send
>>> a reply *twice* !
>> In my defense, I was assuming if the application uses the sendReply()
>> method, it must return a special return value: DO_NOT_REPLY (in
>> RequestHandler interface).
>> This return value is automatically ignored:
>>
>> public final void sendReply(Object reply, boolean exceptionThrown) {
>> if(!header.rsp_expected || reply == RequestHandler.DO_NOT_REPLY)
>> return;
> OK
>
>
>>> A few changes I have in mind (need to think about it more):
>>>
>>> - I want to leave the existing RequestHandler interface in place, so
>>> current implementation continue to work
>>> - There will be a new AsyncRequestHandler interface (possibly extending
>>> RequestHandler, so an implementation can decide to implement both). The
>>> RequestCorrelator needs to have either request_handler or
>>> async_request_handler set. If the former is set, the logic is unchanged.
>>> If the latter is set I'll invoke the async dispatching code
>> I'm not sure if it is a good idea to have the AsyncRequestHandler
>> extending the RequestHandler interface. If the application implements
>> both methods (Object handle(Message) and void handle(Message, ...)) how
>> do you know which method should be invoked?
>
> 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). However, I will change the 
AsyncRequestHandler interface to:

Object handle(Message message, Response response) throws Exception

and the Response interface would be as follow:

void sendResponse(Object reply, boolean isException) //or sendReply(...);

void setAsyncResponse(boolean value); //or setAsyncReply(...);

boolean isAsyncResponse(); //or isAsyncReply();

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.