You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Wilder Rodrigues <wr...@schubergphilis.com> on 2014/02/27 09:03:57 UTC
Review Request 18568: Fix for returning null when expected is boolean;
comparing objects with == instead of equals()
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18568/
-----------------------------------------------------------
Review request for cloudstack, daan Hoogland and Hugo Trippaers.
Repository: cloudstack-git
Description
-------
Fix for Find Bugs findings on troubling issues: returning null when expected is boolean; adding 6 unit tests and fix 1 in the KVMFencer; comparing objects with == instead of equals()
Diffs
-----
plugins/hypervisors/xen/src/com/cloud/ha/XenServerFencer.java 28cba2b
plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java 48ae3ea
plugins/hypervisors/xen/test/com/cloud/ha/XenServerFencerTest.java bd1d8f8
server/src/com/cloud/ha/KVMFencer.java 516a579
server/test/com/cloud/ha/KVMFencerTest.java cdd13b6
Diff: https://reviews.apache.org/r/18568/diff/
Testing
-------
Added 6 unit tests for XenServerFencer (removed an useless one which was testing get/set methods)
Build completed successfully
Tested also on DevCloud + XenServer the following:
Create Zone + Network + Pod + cluster + Pri/Sec Storage + Console Proxy and System VMS
Create 1 instance (tiny Linux) + Guest Network + 1 SourceNAT + 1 extra pub ip
Set up firewall for port 22 + ICMP + port forwarding 22 ==> instance
SSH into the instance
Thanks,
Wilder Rodrigues
Re: Review Request 18568: Fix for returning null when expected is boolean;
comparing objects with == instead of equals()
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18568/#review35946
-----------------------------------------------------------
Ship it!
9045e41483dcedfb57a9b8087bddb5115e9b8a6f
- daan Hoogland
On Feb. 27, 2014, 8:03 a.m., Wilder Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18568/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2014, 8:03 a.m.)
>
>
> Review request for cloudstack, daan Hoogland and Hugo Trippaers.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Fix for Find Bugs findings on troubling issues: returning null when expected is boolean; adding 6 unit tests and fix 1 in the KVMFencer; comparing objects with == instead of equals()
>
>
> Diffs
> -----
>
> plugins/hypervisors/xen/src/com/cloud/ha/XenServerFencer.java 28cba2b
> plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java 48ae3ea
> plugins/hypervisors/xen/test/com/cloud/ha/XenServerFencerTest.java bd1d8f8
> server/src/com/cloud/ha/KVMFencer.java 516a579
> server/test/com/cloud/ha/KVMFencerTest.java cdd13b6
>
> Diff: https://reviews.apache.org/r/18568/diff/
>
>
> Testing
> -------
>
> Added 6 unit tests for XenServerFencer (removed an useless one which was testing get/set methods)
> Build completed successfully
>
> Tested also on DevCloud + XenServer the following:
>
> Create Zone + Network + Pod + cluster + Pri/Sec Storage + Console Proxy and System VMS
> Create 1 instance (tiny Linux) + Guest Network + 1 SourceNAT + 1 extra pub ip
> Set up firewall for port 22 + ICMP + port forwarding 22 ==> instance
> SSH into the instance
>
>
> Thanks,
>
> Wilder Rodrigues
>
>
Re: Review Request 18568: Fix for returning null when expected is boolean;
comparing objects with == instead of equals()
Posted by Wilder Rodrigues <wr...@schubergphilis.com>.
> On Feb. 28, 2014, 10:42 p.m., Laszlo Hornyak wrote:
> > I looked into it, the FenceBuilder.fenceOff() method is called from only one place (HighAvailablityManagerImpl) and right under it checks if the return is null, the returned null is handled as if it was false. Therefore this change is safe and also changing the return value to boolean type would be safe as well.
Thanks for having a look into it, Laszlo. I did the same check and it is covered by the unit tests. :)
I have talked to Hugo about having a bug created on Jira. He said that since it's a FindBugs finding, there is no need to have a bug created. I agree on that, otherwise we will face too much administration work in order to fix such things.
However, I think it has to be well tested and analysed before a review request being created.
- Wilder
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18568/#review35862
-----------------------------------------------------------
On Feb. 27, 2014, 8:03 a.m., Wilder Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18568/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2014, 8:03 a.m.)
>
>
> Review request for cloudstack, daan Hoogland and Hugo Trippaers.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Fix for Find Bugs findings on troubling issues: returning null when expected is boolean; adding 6 unit tests and fix 1 in the KVMFencer; comparing objects with == instead of equals()
>
>
> Diffs
> -----
>
> plugins/hypervisors/xen/src/com/cloud/ha/XenServerFencer.java 28cba2b
> plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java 48ae3ea
> plugins/hypervisors/xen/test/com/cloud/ha/XenServerFencerTest.java bd1d8f8
> server/src/com/cloud/ha/KVMFencer.java 516a579
> server/test/com/cloud/ha/KVMFencerTest.java cdd13b6
>
> Diff: https://reviews.apache.org/r/18568/diff/
>
>
> Testing
> -------
>
> Added 6 unit tests for XenServerFencer (removed an useless one which was testing get/set methods)
> Build completed successfully
>
> Tested also on DevCloud + XenServer the following:
>
> Create Zone + Network + Pod + cluster + Pri/Sec Storage + Console Proxy and System VMS
> Create 1 instance (tiny Linux) + Guest Network + 1 SourceNAT + 1 extra pub ip
> Set up firewall for port 22 + ICMP + port forwarding 22 ==> instance
> SSH into the instance
>
>
> Thanks,
>
> Wilder Rodrigues
>
>
Re: Review Request 18568: Fix for returning null when expected is boolean;
comparing objects with == instead of equals()
Posted by Laszlo Hornyak <la...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18568/#review35862
-----------------------------------------------------------
I looked into it, the FenceBuilder.fenceOff() method is called from only one place (HighAvailablityManagerImpl) and right under it checks if the return is null, the returned null is handled as if it was false. Therefore this change is safe and also changing the return value to boolean type would be safe as well.
- Laszlo Hornyak
On Feb. 27, 2014, 8:03 a.m., Wilder Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18568/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2014, 8:03 a.m.)
>
>
> Review request for cloudstack, daan Hoogland and Hugo Trippaers.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Fix for Find Bugs findings on troubling issues: returning null when expected is boolean; adding 6 unit tests and fix 1 in the KVMFencer; comparing objects with == instead of equals()
>
>
> Diffs
> -----
>
> plugins/hypervisors/xen/src/com/cloud/ha/XenServerFencer.java 28cba2b
> plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java 48ae3ea
> plugins/hypervisors/xen/test/com/cloud/ha/XenServerFencerTest.java bd1d8f8
> server/src/com/cloud/ha/KVMFencer.java 516a579
> server/test/com/cloud/ha/KVMFencerTest.java cdd13b6
>
> Diff: https://reviews.apache.org/r/18568/diff/
>
>
> Testing
> -------
>
> Added 6 unit tests for XenServerFencer (removed an useless one which was testing get/set methods)
> Build completed successfully
>
> Tested also on DevCloud + XenServer the following:
>
> Create Zone + Network + Pod + cluster + Pri/Sec Storage + Console Proxy and System VMS
> Create 1 instance (tiny Linux) + Guest Network + 1 SourceNAT + 1 extra pub ip
> Set up firewall for port 22 + ICMP + port forwarding 22 ==> instance
> SSH into the instance
>
>
> Thanks,
>
> Wilder Rodrigues
>
>
Re: Review Request 18568: Fix for returning null when expected is boolean;
comparing objects with == instead of equals()
Posted by Wilder Rodrigues <wr...@schubergphilis.com>.
> On Feb. 27, 2014, 12:10 p.m., Laszlo Hornyak wrote:
> > In general I believe it makes sense to return false in this case, since it is clear that the host was not fenced.
> >
> > At the same time the reviewers dislike havig formating (finals) in the patches, it might be easier to push it through the process if you could separate the cleanup from the fix. Also, is there a bug url that should be included?
Hi Laszlo,
Concerning the "finals" it was due check-style combined with save actions in my Eclipse, just to make sure the code is according to the CS templates. I understand it kind of make a bit difficult to read the patch, but redoing it would require more effort than pushing it through. We can agree that for the next I will do the formatting first, commit and send the patch. Afterwards, I do the fix, squash the commit and update the patch. Is it a better idea?
Concerning the bug URL, there is none. I just picked up a package and ran find Bugs on it.
- Wilder
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18568/#review35618
-----------------------------------------------------------
On Feb. 27, 2014, 8:03 a.m., Wilder Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18568/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2014, 8:03 a.m.)
>
>
> Review request for cloudstack, daan Hoogland and Hugo Trippaers.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Fix for Find Bugs findings on troubling issues: returning null when expected is boolean; adding 6 unit tests and fix 1 in the KVMFencer; comparing objects with == instead of equals()
>
>
> Diffs
> -----
>
> plugins/hypervisors/xen/src/com/cloud/ha/XenServerFencer.java 28cba2b
> plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java 48ae3ea
> plugins/hypervisors/xen/test/com/cloud/ha/XenServerFencerTest.java bd1d8f8
> server/src/com/cloud/ha/KVMFencer.java 516a579
> server/test/com/cloud/ha/KVMFencerTest.java cdd13b6
>
> Diff: https://reviews.apache.org/r/18568/diff/
>
>
> Testing
> -------
>
> Added 6 unit tests for XenServerFencer (removed an useless one which was testing get/set methods)
> Build completed successfully
>
> Tested also on DevCloud + XenServer the following:
>
> Create Zone + Network + Pod + cluster + Pri/Sec Storage + Console Proxy and System VMS
> Create 1 instance (tiny Linux) + Guest Network + 1 SourceNAT + 1 extra pub ip
> Set up firewall for port 22 + ICMP + port forwarding 22 ==> instance
> SSH into the instance
>
>
> Thanks,
>
> Wilder Rodrigues
>
>
Re: Review Request 18568: Fix for returning null when expected is boolean;
comparing objects with == instead of equals()
Posted by Laszlo Hornyak <la...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18568/#review35618
-----------------------------------------------------------
In general I believe it makes sense to return false in this case, since it is clear that the host was not fenced.
At the same time the reviewers dislike havig formating (finals) in the patches, it might be easier to push it through the process if you could separate the cleanup from the fix. Also, is there a bug url that should be included?
- Laszlo Hornyak
On Feb. 27, 2014, 8:03 a.m., Wilder Rodrigues wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18568/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2014, 8:03 a.m.)
>
>
> Review request for cloudstack, daan Hoogland and Hugo Trippaers.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Fix for Find Bugs findings on troubling issues: returning null when expected is boolean; adding 6 unit tests and fix 1 in the KVMFencer; comparing objects with == instead of equals()
>
>
> Diffs
> -----
>
> plugins/hypervisors/xen/src/com/cloud/ha/XenServerFencer.java 28cba2b
> plugins/hypervisors/xen/src/com/cloud/hypervisor/xen/resource/CitrixResourceBase.java 48ae3ea
> plugins/hypervisors/xen/test/com/cloud/ha/XenServerFencerTest.java bd1d8f8
> server/src/com/cloud/ha/KVMFencer.java 516a579
> server/test/com/cloud/ha/KVMFencerTest.java cdd13b6
>
> Diff: https://reviews.apache.org/r/18568/diff/
>
>
> Testing
> -------
>
> Added 6 unit tests for XenServerFencer (removed an useless one which was testing get/set methods)
> Build completed successfully
>
> Tested also on DevCloud + XenServer the following:
>
> Create Zone + Network + Pod + cluster + Pri/Sec Storage + Console Proxy and System VMS
> Create 1 instance (tiny Linux) + Guest Network + 1 SourceNAT + 1 extra pub ip
> Set up firewall for port 22 + ICMP + port forwarding 22 ==> instance
> SSH into the instance
>
>
> Thanks,
>
> Wilder Rodrigues
>
>