[DISCUSS] Replacing Executor by a new Actor on ServerSessionHandler

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[DISCUSS] Replacing Executor by a new Actor on ServerSessionHandler

clebertsuconic
I am replacing the use of an executor on ServerSessionHandler by a new
class I just wrote (after some deep technical discussion with
Francesco, to give him the credits)..

ServerSessionHandler is currently creating a new Runnable on every
call made through the PacketHandler...

this is because:

of this method:

  @Override
   public void handlePacket(final Packet packet) {
      channel.confirm(packet);
      callExecutor.execute(() -> internalHandlePacket(packet));
   }



The new Actor class is similar to an executor, but it varies on the following:

Instead of receiving a Runnable, it receives an argument (or message)
that is then sent to a listener method:


The implementation gets a lot clearer:


@Override
public void handlePacket(final Packet packet) {
   channel.confirm(packet);

   // This method will call onMessagePacket through an actor
   packetActor.sendMessage(packet);
}



And the method is just called without a new Runnable.



Here's the PR: https://github.com/apache/activemq-artemis/pull/1395




--
Clebert Suconic
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Replacing Executor by a new Actor on ServerSessionHandler

clebertsuconic
Also: I need some help with this.


Actor is defined as Actor<T>


Does anyone know a way to define a method similar to this:


SomeFactory:
  Actor<T> newActor(T type)


That is, type of Actor is passed in as argument. This is because I
would like OrderedExecutorFactory to include the new Actor method, and
I couldn't figure out how to do this through a factory.

On Mon, Jul 10, 2017 at 7:26 PM, Clebert Suconic
<[hidden email]> wrote:

> I am replacing the use of an executor on ServerSessionHandler by a new
> class I just wrote (after some deep technical discussion with
> Francesco, to give him the credits)..
>
> ServerSessionHandler is currently creating a new Runnable on every
> call made through the PacketHandler...
>
> this is because:
>
> of this method:
>
>   @Override
>    public void handlePacket(final Packet packet) {
>       channel.confirm(packet);
>       callExecutor.execute(() -> internalHandlePacket(packet));
>    }
>
>
>
> The new Actor class is similar to an executor, but it varies on the following:
>
> Instead of receiving a Runnable, it receives an argument (or message)
> that is then sent to a listener method:
>
>
> The implementation gets a lot clearer:
>
>
> @Override
> public void handlePacket(final Packet packet) {
>    channel.confirm(packet);
>
>    // This method will call onMessagePacket through an actor
>    packetActor.sendMessage(packet);
> }
>
>
>
> And the method is just called without a new Runnable.
>
>
>
> Here's the PR: https://github.com/apache/activemq-artemis/pull/1395
>
>
>
>
> --
> Clebert Suconic



--
Clebert Suconic
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Replacing Executor by a new Actor on ServerSessionHandler

MichaelAndrePearce
Being maybe naive here, but won't this make the handlePacket method blocking now?

Isn't the idea to do work load hand off to other threads which are in the executor?



> On 11 Jul 2017, at 00:30, Clebert Suconic <[hidden email]> wrote:
>
> Also: I need some help with this.
>
>
> Actor is defined as Actor<T>
>
>
> Does anyone know a way to define a method similar to this:
>
>
> SomeFactory:
>  Actor<T> newActor(T type)
>
>
> That is, type of Actor is passed in as argument. This is because I
> would like OrderedExecutorFactory to include the new Actor method, and
> I couldn't figure out how to do this through a factory.
>
> On Mon, Jul 10, 2017 at 7:26 PM, Clebert Suconic
> <[hidden email]> wrote:
>> I am replacing the use of an executor on ServerSessionHandler by a new
>> class I just wrote (after some deep technical discussion with
>> Francesco, to give him the credits)..
>>
>> ServerSessionHandler is currently creating a new Runnable on every
>> call made through the PacketHandler...
>>
>> this is because:
>>
>> of this method:
>>
>>  @Override
>>   public void handlePacket(final Packet packet) {
>>      channel.confirm(packet);
>>      callExecutor.execute(() -> internalHandlePacket(packet));
>>   }
>>
>>
>>
>> The new Actor class is similar to an executor, but it varies on the following:
>>
>> Instead of receiving a Runnable, it receives an argument (or message)
>> that is then sent to a listener method:
>>
>>
>> The implementation gets a lot clearer:
>>
>>
>> @Override
>> public void handlePacket(final Packet packet) {
>>   channel.confirm(packet);
>>
>>   // This method will call onMessagePacket through an actor
>>   packetActor.sendMessage(packet);
>> }
>>
>>
>>
>> And the method is just called without a new Runnable.
>>
>>
>>
>> Here's the PR: https://github.com/apache/activemq-artemis/pull/1395
>>
>>
>>
>>
>> --
>> Clebert Suconic
>
>
>
> --
> Clebert Suconic
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Replacing Executor by a new Actor on ServerSessionHandler

MichaelAndrePearce
Always helps to fully read the PR code, not just the snippet in the email.

It still is thread handing off, as such ignore my previous comment, and +1 to the general idea.

Just one other question:

"why not use an existing actor framework instead of coding our own, e.g. vert.x or akka?"

I’d be a bit wary of trying to re-implement these, a bit like with why we use Netty.

I note that right now the simple model is fine, but as the idea extends we could end up if not careful coding up a framework, which if we start at the offset using one like vert.x, we could avoid.


Anyhow as said +1 to the general idea.



> On 11 Jul 2017, at 04:46, Michael André Pearce <[hidden email]> wrote:
>
> Being maybe naive here, but won't this make the handlePacket method blocking now?
>
> Isn't the idea to do work load hand off to other threads which are in the executor?
>
>
>
>> On 11 Jul 2017, at 00:30, Clebert Suconic <[hidden email]> wrote:
>>
>> Also: I need some help with this.
>>
>>
>> Actor is defined as Actor<T>
>>
>>
>> Does anyone know a way to define a method similar to this:
>>
>>
>> SomeFactory:
>> Actor<T> newActor(T type)
>>
>>
>> That is, type of Actor is passed in as argument. This is because I
>> would like OrderedExecutorFactory to include the new Actor method, and
>> I couldn't figure out how to do this through a factory.
>>
>> On Mon, Jul 10, 2017 at 7:26 PM, Clebert Suconic
>> <[hidden email]> wrote:
>>> I am replacing the use of an executor on ServerSessionHandler by a new
>>> class I just wrote (after some deep technical discussion with
>>> Francesco, to give him the credits)..
>>>
>>> ServerSessionHandler is currently creating a new Runnable on every
>>> call made through the PacketHandler...
>>>
>>> this is because:
>>>
>>> of this method:
>>>
>>> @Override
>>>  public void handlePacket(final Packet packet) {
>>>     channel.confirm(packet);
>>>     callExecutor.execute(() -> internalHandlePacket(packet));
>>>  }
>>>
>>>
>>>
>>> The new Actor class is similar to an executor, but it varies on the following:
>>>
>>> Instead of receiving a Runnable, it receives an argument (or message)
>>> that is then sent to a listener method:
>>>
>>>
>>> The implementation gets a lot clearer:
>>>
>>>
>>> @Override
>>> public void handlePacket(final Packet packet) {
>>>  channel.confirm(packet);
>>>
>>>  // This method will call onMessagePacket through an actor
>>>  packetActor.sendMessage(packet);
>>> }
>>>
>>>
>>>
>>> And the method is just called without a new Runnable.
>>>
>>>
>>>
>>> Here's the PR: https://github.com/apache/activemq-artemis/pull/1395
>>>
>>>
>>>
>>>
>>> --
>>> Clebert Suconic
>>
>>
>>
>> --
>> Clebert Suconic

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Replacing Executor by a new Actor on ServerSessionHandler

clebertsuconic
On Tue, Jul 11, 2017 at 12:06 AM, Michael André Pearce
<[hidden email]> wrote:
> Always helps to fully read the PR code, not just the snippet in the email.
>
> It still is thread handing off, as such ignore my previous comment, and +1 to the general idea.
>
> Just one other question:
>
> "why not use an existing actor framework instead of coding our own, e.g. vert.x or akka?"
>
> I’d be a bit wary of trying to re-implement these, a bit like with why we use Netty.


the idea is just to replace the executor for something simple.. that
won't happen.. I don't have that time on my hands anyway :)
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [DISCUSS] Replacing Executor by a new Actor on ServerSessionHandler

clebertsuconic
I just blogged about this:
https://blogs.apache.org/activemq/entry/actor-and-executors

On Tue, Jul 11, 2017 at 8:55 AM, Clebert Suconic
<[hidden email]> wrote:

> On Tue, Jul 11, 2017 at 12:06 AM, Michael André Pearce
> <[hidden email]> wrote:
>> Always helps to fully read the PR code, not just the snippet in the email.
>>
>> It still is thread handing off, as such ignore my previous comment, and +1 to the general idea.
>>
>> Just one other question:
>>
>> "why not use an existing actor framework instead of coding our own, e.g. vert.x or akka?"
>>
>> I’d be a bit wary of trying to re-implement these, a bit like with why we use Netty.
>
>
> the idea is just to replace the executor for something simple.. that
> won't happen.. I don't have that time on my hands anyway :)



--
Clebert Suconic
Loading...