All,
I've made the following changes to public Snapshot takeSnapshot(Long
volumeId, Long policyId, Long snapshotId, Account account, boolean
quiescevm), is this the preferred approach?
I don't get any compilation issues with this. I want to ensure I've taken
your comments on fully.
try
{
CreateSnapshotPayload payload = new CreateSnapshotPayload();
payload.setSnapshotId(snapshotId);
payload.setSnapshotPolicyId(policyId);
payload.setAccount(account);
payload.setQuiescevm(quiescevm);
volume.addPayload(payload);
SnapshotInfo snapshotInfoReturn = volService.takeSnapshot(volume);
if(snapshotInfoReturn.equals(null)){
throw new ResourceAllocationException("Take snapshot for VolumeId: "
+ volumeId + " failed.", ResourceType.snapshot);
}
return volService.takeSnapshot(volume);
}
catch(ResourceAllocationException res){
throw new ResourceAllocationException("Take snapshot for VolumeId: "
+ volumeId + " failed.", ResourceType.snapshot);
}
Alex Hitchins | 07788 423 969 | 01892 523 587
---------------------------------------------
-----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
