You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Patrick Hunt <ph...@apache.org> on 2011/12/15 00:36:06 UTC
Re: Review Request: QuorumPeer should unregister only its previsously
registered MBeans instead of use MBeanRegistry.unregisterAll() method.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2208/#review3914
-----------------------------------------------------------
a good cleanup, some comments before we can commit it.
/src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java
<https://reviews.apache.org/r/2208/#comment8794>
cleanup this typo (2 returns)
/src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java
<https://reviews.apache.org/r/2208/#comment8793>
let's call this
getRegisteredBeans()
/src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java
<https://reviews.apache.org/r/2208/#comment8795>
Add back the try/catch protection. This was necessary in some cases (I can't remember why off the top of my head though).
/src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java
<https://reviews.apache.org/r/2208/#comment8796>
license header is missing. add the std boilerplate found on any other java file in the project.
/src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java
<https://reviews.apache.org/r/2208/#comment8798>
add a comment to the class describing what this set of tests is for
/src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java
<https://reviews.apache.org/r/2208/#comment8797>
no tabs, reformat with spaces only. (you'll probably need to cleanup some of your other changes at the same time)
/src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java
<https://reviews.apache.org/r/2208/#comment8799>
it's fine to ref a jira here, however there should also be some details on what this test is doing. (short summary esp as the test itself is pretty clear)
- Patrick
On 2011-10-05 11:59:30, César Álvarez Núñez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2208/
> -----------------------------------------------------------
>
> (Updated 2011-10-05 11:59:30)
>
>
> Review request for zookeeper.
>
>
> Summary
> -------
>
> See https://issues.apache.org/jira/browse/ZOOKEEPER-1214
>
>
> This addresses bug ZOOKEEPER-1214.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1214
>
>
> Diffs
> -----
>
> /src/java/test/org/apache/zookeeper/test/QuorumUtil.java 1179165
> /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java PRE-CREATION
> /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1179166
> /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java 1169669
>
> Diff: https://reviews.apache.org/r/2208/diff
>
>
> Testing
> -------
>
> -
>
>
> Thanks,
>
> César
>
>
Re: Review Request: QuorumPeer should unregister only its previsously
registered MBeans instead of use MBeanRegistry.unregisterAll() method.
Posted by César Álvarez Núñez <ce...@gmail.com>.
> On 2011-12-14 23:36:06, Patrick Hunt wrote:
> > /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java, lines 126-127
> > <https://reviews.apache.org/r/2208/diff/1/?file=48123#file48123line126>
> >
> > cleanup this typo (2 returns)
Done.
> On 2011-12-14 23:36:06, Patrick Hunt wrote:
> > /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java, line 130
> > <https://reviews.apache.org/r/2208/diff/1/?file=48123#file48123line130>
> >
> > let's call this
> >
> > getRegisteredBeans()
> >
Ok.
> On 2011-12-14 23:36:06, Patrick Hunt wrote:
> > /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, lines 757-767
> > <https://reviews.apache.org/r/2208/diff/1/?file=48124#file48124line757>
> >
> > Add back the try/catch protection. This was necessary in some cases (I can't remember why off the top of my head though).
I've enhanced the exception handling during "public void unregister(ZKMBeanInfo bean)" method invocation since JMException was caught and logged two times: 1) private void unregister(String path,ZKMBeanInfo bean) throws JMException, 2) public void unregister(ZKMBeanInfo bean).
I've also added a "catch(Throwable t)" to "public void unregister(ZKMBeanInfo bean)" method that will generate an error log instead of a warn.
> On 2011-12-14 23:36:06, Patrick Hunt wrote:
> > /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java, line 1
> > <https://reviews.apache.org/r/2208/diff/1/?file=48126#file48126line1>
> >
> > license header is missing. add the std boilerplate found on any other java file in the project.
As you can see I'm a newbie contributor ;-)
> On 2011-12-14 23:36:06, Patrick Hunt wrote:
> > /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java, line 15
> > <https://reviews.apache.org/r/2208/diff/1/?file=48126#file48126line15>
> >
> > add a comment to the class describing what this set of tests is for
Done.
> On 2011-12-14 23:36:06, Patrick Hunt wrote:
> > /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java, line 16
> > <https://reviews.apache.org/r/2208/diff/1/?file=48126#file48126line16>
> >
> > no tabs, reformat with spaces only. (you'll probably need to cleanup some of your other changes at the same time)
Done. It would be nice that the eclipse ant task will create the .setting files to set all formatting rules and useful save actions (like apply formatting). I'll investigate it and upload as a new jira if success.
> On 2011-12-14 23:36:06, Patrick Hunt wrote:
> > /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java, line 19
> > <https://reviews.apache.org/r/2208/diff/1/?file=48126#file48126line19>
> >
> > it's fine to ref a jira here, however there should also be some details on what this test is doing. (short summary esp as the test itself is pretty clear)
Done.
- César
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2208/#review3914
-----------------------------------------------------------
On 2011-10-05 11:59:30, César Álvarez Núñez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2208/
> -----------------------------------------------------------
>
> (Updated 2011-10-05 11:59:30)
>
>
> Review request for zookeeper.
>
>
> Summary
> -------
>
> See https://issues.apache.org/jira/browse/ZOOKEEPER-1214
>
>
> This addresses bug ZOOKEEPER-1214.
> https://issues.apache.org/jira/browse/ZOOKEEPER-1214
>
>
> Diffs
> -----
>
> /src/java/test/org/apache/zookeeper/test/QuorumUtil.java 1179165
> /src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java PRE-CREATION
> /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1179166
> /src/java/main/org/apache/zookeeper/jmx/MBeanRegistry.java 1169669
>
> Diff: https://reviews.apache.org/r/2208/diff
>
>
> Testing
> -------
>
> -
>
>
> Thanks,
>
> César
>
>