Review Board 1.7.22

Zookeeper#delete , #create - async versions miss a verb in the javadoc

Review Request #1715 - Created Sept. 5, 2011 and submitted

Thomas Koch

Posted (Sept. 5, 2011, 7:17 p.m.)


Is there some reason you moved this LOG initialization as part of this patch? I don't think there's anything wrong with it but it does seem unrelated.
Posted (Sept. 5, 2011, 7:43 p.m.)


it is unrelated to the rest of the patch, but I touched the file anyway and it's an obvious clean up that follows the boy scout rule.[1]

The previous LOG initialization was very confusing. It made me think whether there's some JVM magic that would require LOG to be initialized in the static block rather then in the normal way.

  1. Ok, seems reasonable. +1, I'll check this in to trunk.
Posted (Sept. 5, 2011, 8:04 p.m.)


src/java/main/org/apache/zookeeper/ (Diff revision 1)
I suspect LOG is initialized in the static to make initialization explicit - given logEnv uses LOG as an argument. We'll lose this with this change, i.e. someone might re-refactor the code and lose the implicit ordering. At the very least it would be a good idea to document.
  1. Pat,
    I checked this into head already. Would you prefer I roll back this part of the change or just add a comment?
  2. imo putting the initialization of LOG back into the static would be the right thing to do, but feel free to just add the comment. This is a small/discussed change, if you want you could a) just submit a second patch to the jira that moves the init back into the static and adds a comment on why we do it this way, then  go ahead and commit that change. I don't believe we'd need to go through a more lengthy review...
  3. Sounds good, will do.
  4. Cool. 
    Btw, I do agree with Thomas this is a code smell - we are trying to output the environment detail once, when the first ZK client is instantiated. Perhaps this code just needs more heavy refactoring, i.e. rather than hanging logEnv in the static perhaps there's a better way?