You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "James Baldassari (Commented) (JIRA)" <ji...@apache.org> on 2012/01/22 23:04:40 UTC
[jira] [Commented] (AVRO-1001) Adding thread pool to
NettyServerAvroHandler
[ https://issues.apache.org/jira/browse/AVRO-1001?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13190801#comment-13190801 ]
James Baldassari commented on AVRO-1001:
----------------------------------------
Thanks for the patch. I was surprised to see this issue because when the NettyServer is initialized it actually passes two different thread pools to the NioServerSocketChannelFactory:
{code}
public NettyServer(Responder responder, InetSocketAddress addr) {
this(responder, addr, new NioServerSocketChannelFactory
(Executors .newCachedThreadPool(), Executors.newCachedThreadPool()));
}
{code}
The first thread pool is for the Netty "boss" threads, and the second is for the Netty "worker" threads. When I dug into the Netty code I found that Netty only starts a single pair of boss and worker threads for each Netty channel. The worker thread sits in a for (;; ) loop selecting on the channel and then processing each message it receives in order. So you're correct that only one RPC can be executing concurrently on a per-channel basis. I was also able to reproduce this issue in a unit test.
Given how Netty works, one work-around is to create a new client for each concurrent RPC that you want to execute. This is not an ideal long-term solution, and I think your idea to add a new thread pool in NettyServer is a good fix. However, the cached thread pool might not be the best implementation for all use cases. For example, cached thread pools can be problematic in high-throughput environments because they are unbounded by definition. So let's allow the ExecutorService to be passed into the NettyServer constructor if the user wants to specify a different/custom implementation. As the default implementation, the cached thread pool should be fine, as long as it can be changed if necessary.
I'll add my unit test to your patch to verify that this change fixes the issue.
> Adding thread pool to NettyServerAvroHandler
> --------------------------------------------
>
> Key: AVRO-1001
> URL: https://issues.apache.org/jira/browse/AVRO-1001
> Project: Avro
> Issue Type: Improvement
> Components: java
> Affects Versions: 1.6.1
> Reporter: Shaun Williams
> Labels: patch
> Attachments: AVRO-1001.patch
>
>
> Request code review.
> The current NettyServer implementation processes each request/response sequentially on a single channel. Thus in the case where a second request is received from the same client while a request is still being processed, the behavior is undefined.
> This patch updates NettyServerAvroHandler.messageReceived() to process each request in a separate thread using an ExecutorService.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
Re: [jira] [Commented] (AVRO-1001) Adding thread pool to
NettyServerAvroHandler
Posted by Shaun Williams <sh...@apple.com>.
Great, I'll give it a try. Thanks!
-Shaun
On Jan 22, 2012, at 2:04 PM, James Baldassari (Commented) (JIRA) wrote:
>
> [ https://issues.apache.org/jira/browse/AVRO-1001?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13190801#comment-13190801 ]
>
> James Baldassari commented on AVRO-1001:
> ----------------------------------------
>
> Thanks for the patch. I was surprised to see this issue because when the NettyServer is initialized it actually passes two different thread pools to the NioServerSocketChannelFactory:
>
> {code}
> public NettyServer(Responder responder, InetSocketAddress addr) {
> this(responder, addr, new NioServerSocketChannelFactory
> (Executors .newCachedThreadPool(), Executors.newCachedThreadPool()));
> }
> {code}
>
> The first thread pool is for the Netty "boss" threads, and the second is for the Netty "worker" threads. When I dug into the Netty code I found that Netty only starts a single pair of boss and worker threads for each Netty channel. The worker thread sits in a for (;; ) loop selecting on the channel and then processing each message it receives in order. So you're correct that only one RPC can be executing concurrently on a per-channel basis. I was also able to reproduce this issue in a unit test.
>
> Given how Netty works, one work-around is to create a new client for each concurrent RPC that you want to execute. This is not an ideal long-term solution, and I think your idea to add a new thread pool in NettyServer is a good fix. However, the cached thread pool might not be the best implementation for all use cases. For example, cached thread pools can be problematic in high-throughput environments because they are unbounded by definition. So let's allow the ExecutorService to be passed into the NettyServer constructor if the user wants to specify a different/custom implementation. As the default implementation, the cached thread pool should be fine, as long as it can be changed if necessary.
>
> I'll add my unit test to your patch to verify that this change fixes the issue.
>
>> Adding thread pool to NettyServerAvroHandler
>> --------------------------------------------
>>
>> Key: AVRO-1001
>> URL: https://issues.apache.org/jira/browse/AVRO-1001
>> Project: Avro
>> Issue Type: Improvement
>> Components: java
>> Affects Versions: 1.6.1
>> Reporter: Shaun Williams
>> Labels: patch
>> Attachments: AVRO-1001.patch
>>
>>
>> Request code review.
>> The current NettyServer implementation processes each request/response sequentially on a single channel. Thus in the case where a second request is received from the same client while a request is still being processed, the behavior is undefined.
>> This patch updates NettyServerAvroHandler.messageReceived() to process each request in a separate thread using an ExecutorService.
>
> --
> This message is automatically generated by JIRA.
> If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
> For more information on JIRA, see: http://www.atlassian.com/software/jira
>
>