That's too much common sense for me to cope with. Thanks for the tip!
-----Original Message----- From: Daan Hoogland [mailto:daan.hoogl...@gmail.com] Sent: 03 April 2014 17:10 To: dev Cc: Alex Hitchins; Edison Su; Laszlo Hornyak Subject: Re: Review Request 19616: Added check for null return. Prasanna made a nice remark about that; look at the annotations and/or git log and see who did the most changes in the code you want to protect the world against. Those are the people to address. On Thu, Apr 3, 2014 at 6:06 PM, Alex Hitchins <a...@alexhitchins.com> wrote: > 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 > -- Daan
