[ 
https://issues.apache.org/jira/browse/BOOKKEEPER-555?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13575352#comment-13575352
 ] 

Rakesh R commented on BOOKKEEPER-555:
-------------------------------------

{quote}-1 the patch seems to introduce 1 new Findbugs warning(s) in module(s) 
[bookkeeper-server]
 {quote}
I think equals overriding will not cause any issues, its giving warning as 
CleanupChannelGroup has new field 'closed' defined.

Hi Ivan/Sijie, Sorry to pitch in late as I got free time today only. 
By the way netty impl looks very nice. Just have few suggestions, could you 
please go through. As there is no functional problems, feel free riasing 
another jira to fix valid comments and go ahead with committing the current 
patch.

- @ChannelPipelineCoverage annotation has been deprecated
and use the alternative @Sharable annotation

- Creates a new group with the specified name in CleanupChannelGroup like, 
rather than using the generated name.
DefaultChannelGroup(){
   super("bookieServerCnxns");
}

- I feel BookieRequestHandler can override channelDisconnected, 
channelConnected and do logging, will make debug much easier in  fluctuating 
env.

- Haven't seen the following var is used anywhere in BookieRequestHandler, 
please remove.
private final ServerConfiguration conf;

- 'boolean success = false;' in BookieRequestHandler is not required and please 
remove it, as read logic is using 'errorCode == BookieProtocol.EOK' to 
differentiate success/failure.

- please organize imports for the classes:
BookieNettyServer, BookieServer, BookieServerBean
                
> Make BookieServer use Netty rather than a custom IO server
> ----------------------------------------------------------
>
>                 Key: BOOKKEEPER-555
>                 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-555
>             Project: Bookkeeper
>          Issue Type: Bug
>            Reporter: Ivan Kelly
>            Assignee: Ivan Kelly
>             Fix For: 4.3.0
>
>         Attachments: 0002-BOOKKEEPER-555-Netty-Server-for-Bookie.patch, 
> 0002-BOOKKEEPER-555-Netty-Server-for-Bookie.patch, 
> 0002-BOOKKEEPER-555-Netty-Server-for-Bookie.patch, BOOKKEEPER-555.patch
>
>
> Move from the custom NIO server to netty. This will make it easier to do 
> things like add more server side threads and support SSL.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Reply via email to