[infinispan-dev] Threadpools in a large cluster

[Dan Berindei]

On Thu, Feb 7, 2013 at 3:55 PM, Pedro Ruivo <pruivo at gsd.inesc-id.pt> wrote:

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

I'd go for 2 references as well, I think the null check is easier for the
JIT/CPU to optimize.

I also think the two interfaces represent two different use cases, so it
should be clear to a reader that a class that implements
AsyncRequestHandler really is using async delivery.


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.
>
>
I think having just one method with two completely different usages is too
confusing. If an AsyncRequestHandler wants to process the message
synchronously, all it has to do is process it and then call
Response.sendReply().

In my view, MessageDispatcher and RpcDispatcher should move to
AsyncRequestHandler and stop implementing RequestHandler (to make it clear
that handle(Message, Response) is called). Their handle(Message, Response)
method should still call handle(Message) on the same thread, for backwards
compatibility, but our CommandAwareRpcDispatcher should override it to
dispatch the message to our own thread pool (or execute it normally,
depending on the command).



> 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.
>
>
A duplicate response should generate an exception on the caller, because
the request is already completed. So I'd rather not add another
synchronization here, but I'd be ok with it either way.


> 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.
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/infinispan-dev/attachments/20130207/cc399203/attachment.html