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
