[jira] [Commented] (BOOKKEEPER-170) Bookie constructor starts a number of threads

["Flavio Junqueira (Commented) (JIRA)" <ji...@apache.org>]

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

Flavio Junqueira commented on BOOKKEEPER-170:
---------------------------------------------

It looks good overall. A few comments:

# I'm not sure why you removed a number of calls to shutdown. Could you explain?
# In Bookie, I thought that we were setting the value of this.zk further down in the constructor because we needed to wait until the bookie starts up. With this patch, we set it earlier. I'm trying to understand why that is not a problem...
# This change here "@@ -346,12 +350,11" does not seem to be necessary.
# This is minor, but I was wondering if we should call startGC() something else, like initiate(). It leaks out information about EntryLogger.
                
> Bookie constructor starts a number of threads
> ---------------------------------------------
>
>                 Key: BOOKKEEPER-170
>                 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-170
>             Project: Bookkeeper
>          Issue Type: Bug
>            Reporter: Ivan Kelly
>            Assignee: Ivan Kelly
>             Fix For: 4.1.0
>
>         Attachments: BOOKKEEPER-170.diff
>
>
> Starting a thread in a constructor is bad[1]. Also, it makes unit testing on Bookie a bit of a pain. For this reason, i've refactored the thread starting code out, so that to start the bookie, you call start() like you usually have to for a thread anyhow. As a bonus, it fixes some findbugs issues.
> [1] http://stackoverflow.com/questions/84285/calling-thread-start-within-its-own-constructor

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