> 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 > >
