You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Thomas Koch <th...@koch.ro> on 2011/09/05 17:55:47 UTC
Review Request: Zookeeper#delete ,
#create - async versions miss a verb in the javadoc
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1715/
-----------------------------------------------------------
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
Re: Review Request: Zookeeper#delete ,
#create - async versions miss a verb in the javadoc
Posted by Camille Fournier <sk...@gmail.com>.
> 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...
Sounds good, will do.
- Camille
-----------------------------------------------------------
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
>
>
Re: Review Request: Zookeeper#delete ,
#create - async versions miss a verb in the javadoc
Posted by Patrick Hunt <ph...@apache.org>.
> 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
>
>
Re: Review Request: Zookeeper#delete ,
#create - async versions miss a verb in the javadoc
Posted by Camille Fournier <sk...@gmail.com>.
> 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.
Pat,
I checked this into head already. Would you prefer I roll back this part of the change or just add a comment?
- Camille
-----------------------------------------------------------
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
>
>
Re: Review Request: Zookeeper#delete ,
#create - async versions miss a verb in the javadoc
Posted by Patrick Hunt <ph...@apache.org>.
> 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?
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...
- 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
>
>
Re: Review Request: Zookeeper#delete ,
#create - async versions miss a verb in the javadoc
Posted by Patrick Hunt <ph...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1715/#review1753
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/ZooKeeper.java
<https://reviews.apache.org/r/1715/#comment3983>
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.
- Patrick
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
>
>
Re: Review Request: Zookeeper#delete ,
#create - async versions miss a verb in the javadoc
Posted by Camille Fournier <sk...@gmail.com>.
> On 2011-09-05 19:43:32, Thomas Koch wrote:
> > src/java/main/org/apache/zookeeper/ZooKeeper.java, line 90
> > <https://reviews.apache.org/r/1715/diff/1/?file=37951#file37951line90>
> >
> > 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] http://pragmaticcraftsman.com/2011/03/the-boy-scout-rule/
Ok, seems reasonable. +1, I'll check this in to trunk.
- Camille
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1715/#review1751
-----------------------------------------------------------
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
>
>
Re: Review Request: Zookeeper#delete ,
#create - async versions miss a verb in the javadoc
Posted by Thomas Koch <th...@koch.ro>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1715/#review1751
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/ZooKeeper.java
<https://reviews.apache.org/r/1715/#comment3981>
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] http://pragmaticcraftsman.com/2011/03/the-boy-scout-rule/
- Thomas
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
>
>
Re: Review Request: Zookeeper#delete ,
#create - async versions miss a verb in the javadoc
Posted by Camille Fournier <sk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1715/#review1750
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/ZooKeeper.java
<https://reviews.apache.org/r/1715/#comment3980>
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.
- Camille
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
>
>