Thanks for looking Daan, I'm unsure sometimes who is best to add to review certain projects/sections.
I will review your comments and look out for those of Edison/Laszlo. My immediate concern was to stop the log saying it had succeeded when it hadn't in fact succeeded. Again, I'll review this and make amendments. Alex Hitchins -----Original Message----- From: Daan Hoogland [mailto:daan.hoogl...@gmail.com] Sent: 03 April 2014 16:53 To: dev; Alex Hitchins; Edison Su; Laszlo Hornyak Subject: Re: Review Request 19616: Added check for null return. H Alex, I had a quick look, keeping Laszlo's remark in mind. volService.takeSnapshot(volume) catches all exceptions and then returns null if any is caught. All calls are from tests or from VolumeApiServieImpl so I think we can safely push the exeption handler down and not return null from takeSnapshot. The only issue is that the VolumeService interface should then throw it. (adding Edison in recip list) you want to do this: throw new ResourceAllocationException("Take snapshot for VolumeId: " + volumeId + " failed.", ResourceType.snapshot); after calling @Override public SnapshotInfo takeSnapshot(VolumeInfo volume) { SnapshotInfo snapshot = null; try { snapshot = snapshotMgr.takeSnapshot(volume); } catch (Exception e) { s_logger.debug("Take snapshot: " + volume.getId() + " failed", e); } return snapshot; } the only Exception thrown in that method is a ResourceAllocationException. I think it is better to let that one go through or handle the error. thoughts Laszlo, Edison? If you want people to look at your review reqest you can add them to the list of reviewers in revoew board. Regards, Daan On Thu, Apr 3, 2014 at 2:24 PM, Alex Hitchins <alex.hitch...@shapeblue.com> wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19616/ > ----------------------------------------------------------- > > (Updated April 3, 2014, 12:24 p.m.) > > > Review request for cloudstack. > > > Changes > ------- > > Daan, are you able to take a look at this for me? > > > Repository: cloudstack-git > > > Description > ------- > > Added check for returned null, if received then throw exception. > > > Diffs > ----- > > server/src/com/cloud/storage/VolumeApiServiceImpl.java 5ffa99b > > Diff: https://reviews.apache.org/r/19616/diff/ > > > Testing > ------- > > Compiled & ran. > > > Thanks, > > Alex Hitchins > -- Daan
