[infinispan-dev] Threadpools in a large cluster

[Bela Ban]

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.

>> - 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.

-- 
Bela Ban, JGroups lead (http://www.jgroups.org)