> On 2011-09-05 20:04:12, Patrick Hunt wrote: > > src/java/main/org/apache/zookeeper/ZooKeeper.java, lines 90-94 > > <https://reviews.apache.org/r/1715/diff/1/?file=37951#file37951line90> > > > > 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. > > Camille Fournier wrote: > Pat, > I checked this into head already. Would you prefer I roll back this part > of the change or just add a comment? > > Patrick Hunt wrote: > 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... > > Camille Fournier wrote: > Sounds good, will do.
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? - Patrick ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1715/#review1753 ----------------------------------------------------------- On 2011-09-05 15:55:47, Thomas Koch wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1715/ > ----------------------------------------------------------- > > (Updated 2011-09-05 15:55:47) > > > Review request for zookeeper. > > > Summary > ------- > > . > > > This addresses bug ZOOKEEPER-731. > https://issues.apache.org/jira/browse/ZOOKEEPER-731 > > > Diffs > ----- > > src/java/main/org/apache/zookeeper/ZooKeeper.java 00bac9f > > Diff: https://reviews.apache.org/r/1715/diff > > > Testing > ------- > > > Thanks, > > Thomas > >
