You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Alex Hitchins <al...@shapeblue.com> on 2014/03/25 16:04:25 UTC
Review Request 19616: Added check for null return.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19616/
-----------------------------------------------------------
Review request for cloudstack.
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
Re: Review Request 19616: Added check for null return.
Posted by Sebastien Goasguen <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19616/#review79042
-----------------------------------------------------------
Thank you for submitting your CloudStack contribution through review board. After discussion on the dev@cloudstack.apache.org the community decided to close down review board and start accepting contributiong through GitHub pull requests. We have been using GH PR for several months now and the process is better than review board.
We will keep Review Board open for another week to give you time to migrate your patch to a github PR if you wish. After that time, your patch will no longer be viewable (even though it will not be deleted).
Please consider submitting a pull request.
Great instructions are available at:
https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md
Thank you very much for your time and your contribution to Apache CloudStack, we hope that using this new process will encourage you to do more.
- Sebastien Goasguen
On July 1, 2014, 3:29 p.m., Alex Hitchins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19616/
> -----------------------------------------------------------
>
> (Updated July 1, 2014, 3:29 p.m.)
>
>
> Review request for cloudstack.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Added check for returned null, if received then throw exception.
>
> Amendments made as per dev mailing list comments.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/VolumeApiServiceImpl.java 680cd2e
>
> Diff: https://reviews.apache.org/r/19616/diff/
>
>
> Testing
> -------
>
> Compiled & ran.
>
>
> Thanks,
>
> Alex Hitchins
>
>
Re: Review Request 19616: Added check for null return.
Posted by Alex Hitchins <al...@shapeblue.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19616/
-----------------------------------------------------------
(Updated July 1, 2014, 3:29 p.m.)
Review request for cloudstack.
Changes
-------
not discarding as you might want to fix it yet. please consider discarding yourself.
Repository: cloudstack-git
Description
-------
Added check for returned null, if received then throw exception.
Amendments made as per dev mailing list comments.
Diffs
-----
server/src/com/cloud/storage/VolumeApiServiceImpl.java 680cd2e
Diff: https://reviews.apache.org/r/19616/diff/
Testing
-------
Compiled & ran.
Thanks,
Alex Hitchins
RE: Review Request 19616: Added check for null return.
Posted by Alex Hitchins <al...@alexhitchins.com>.
Sorry - I picked that up but must have been after I'd done the patch. Will amend _again_!
Alex Hitchins | 07788 423 969 | 01892 523 587
---------------------------------------------
-----Original Message-----
From: daan Hoogland [mailto:noreply@reviews.apache.org] On Behalf Of daan Hoogland
Sent: 15 April 2014 15:07
To: daan Hoogland
Cc: Alex Hitchins; cloudstack
Subject: Re: Review Request 19616: Added check for null return.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19616/#review40390
-----------------------------------------------------------
server/src/com/cloud/storage/VolumeApiServiceImpl.java
<https://reviews.apache.org/r/19616/#comment73373>
calling equals(null) on an object will never return true. It will throw a npe or return false. Do you mean to fix this?
- daan Hoogland
On April 15, 2014, 10:18 a.m., Alex Hitchins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19616/
> -----------------------------------------------------------
>
> (Updated April 15, 2014, 10:18 a.m.)
>
>
> Review request for cloudstack and daan Hoogland.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Added check for returned null, if received then throw exception.
>
> Amendments made as per dev mailing list comments.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/VolumeApiServiceImpl.java 680cd2e
>
> Diff: https://reviews.apache.org/r/19616/diff/
>
>
> Testing
> -------
>
> Compiled & ran.
>
>
> Thanks,
>
> Alex Hitchins
>
>
Re: Review Request 19616: Added check for null return.
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19616/#review40390
-----------------------------------------------------------
server/src/com/cloud/storage/VolumeApiServiceImpl.java
<https://reviews.apache.org/r/19616/#comment73373>
calling equals(null) on an object will never return true. It will throw a npe or return false. Do you mean to fix this?
- daan Hoogland
On April 15, 2014, 10:18 a.m., Alex Hitchins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19616/
> -----------------------------------------------------------
>
> (Updated April 15, 2014, 10:18 a.m.)
>
>
> Review request for cloudstack and daan Hoogland.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Added check for returned null, if received then throw exception.
>
> Amendments made as per dev mailing list comments.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/VolumeApiServiceImpl.java 680cd2e
>
> Diff: https://reviews.apache.org/r/19616/diff/
>
>
> Testing
> -------
>
> Compiled & ran.
>
>
> Thanks,
>
> Alex Hitchins
>
>
RE: Review Request 19616: Added check for null return.
Posted by Alex Hitchins <al...@alexhitchins.com>.
I will take a look later tonight.
Alex Hitchins | 07788 423 969 | 01892 523 587
-----Original Message-----
From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
Sent: 10 June 2014 16:01
To: dev
Subject: Re: Review Request 19616: Added check for null return.
it wasn't addressed in the uploaded patch.
On Tue, Jun 10, 2014 at 4:59 PM, Alex Hitchins <al...@alexhitchins.com> wrote:
> I believe it to be valid still. I'll review later and confirm. I know you had comments on the patch which I had addressed.
>
>
>
>
>> On 10 Jun 2014, at 15:50, "daan Hoogland" <da...@gmail.com> wrote:
>>
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/19616/#review45234
>> -----------------------------------------------------------
>>
>>
>> is this still a valid patch? should it be deleted?
>>
>> - daan Hoogland
>>
>>
>>> On April 15, 2014, 10:18 a.m., Alex Hitchins wrote:
>>>
>>> -----------------------------------------------------------
>>> This is an automatically generated e-mail. To reply, visit:
>>> https://reviews.apache.org/r/19616/
>>> -----------------------------------------------------------
>>>
>>> (Updated April 15, 2014, 10:18 a.m.)
>>>
>>>
>>> Review request for cloudstack and daan Hoogland.
>>>
>>>
>>> Repository: cloudstack-git
>>>
>>>
>>> Description
>>> -------
>>>
>>> Added check for returned null, if received then throw exception.
>>>
>>> Amendments made as per dev mailing list comments.
>>>
>>>
>>> Diffs
>>> -----
>>>
>>> server/src/com/cloud/storage/VolumeApiServiceImpl.java 680cd2e
>>>
>>> Diff: https://reviews.apache.org/r/19616/diff/
>>>
>>>
>>> Testing
>>> -------
>>>
>>> Compiled & ran.
>>>
>>>
>>> Thanks,
>>>
>>> Alex Hitchins
>>
--
Daan
Re: Review Request 19616: Added check for null return.
Posted by Daan Hoogland <da...@gmail.com>.
it wasn't addressed in the uploaded patch.
On Tue, Jun 10, 2014 at 4:59 PM, Alex Hitchins <al...@alexhitchins.com> wrote:
> I believe it to be valid still. I'll review later and confirm. I know you had comments on the patch which I had addressed.
>
>
>
>
>> On 10 Jun 2014, at 15:50, "daan Hoogland" <da...@gmail.com> wrote:
>>
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/19616/#review45234
>> -----------------------------------------------------------
>>
>>
>> is this still a valid patch? should it be deleted?
>>
>> - daan Hoogland
>>
>>
>>> On April 15, 2014, 10:18 a.m., Alex Hitchins wrote:
>>>
>>> -----------------------------------------------------------
>>> This is an automatically generated e-mail. To reply, visit:
>>> https://reviews.apache.org/r/19616/
>>> -----------------------------------------------------------
>>>
>>> (Updated April 15, 2014, 10:18 a.m.)
>>>
>>>
>>> Review request for cloudstack and daan Hoogland.
>>>
>>>
>>> Repository: cloudstack-git
>>>
>>>
>>> Description
>>> -------
>>>
>>> Added check for returned null, if received then throw exception.
>>>
>>> Amendments made as per dev mailing list comments.
>>>
>>>
>>> Diffs
>>> -----
>>>
>>> server/src/com/cloud/storage/VolumeApiServiceImpl.java 680cd2e
>>>
>>> Diff: https://reviews.apache.org/r/19616/diff/
>>>
>>>
>>> Testing
>>> -------
>>>
>>> Compiled & ran.
>>>
>>>
>>> Thanks,
>>>
>>> Alex Hitchins
>>
--
Daan
Re: Review Request 19616: Added check for null return.
Posted by Alex Hitchins <al...@alexhitchins.com>.
I believe it to be valid still. I'll review later and confirm. I know you had comments on the patch which I had addressed.
> On 10 Jun 2014, at 15:50, "daan Hoogland" <da...@gmail.com> wrote:
>
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19616/#review45234
> -----------------------------------------------------------
>
>
> is this still a valid patch? should it be deleted?
>
> - daan Hoogland
>
>
>> On April 15, 2014, 10:18 a.m., Alex Hitchins wrote:
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/19616/
>> -----------------------------------------------------------
>>
>> (Updated April 15, 2014, 10:18 a.m.)
>>
>>
>> Review request for cloudstack and daan Hoogland.
>>
>>
>> Repository: cloudstack-git
>>
>>
>> Description
>> -------
>>
>> Added check for returned null, if received then throw exception.
>>
>> Amendments made as per dev mailing list comments.
>>
>>
>> Diffs
>> -----
>>
>> server/src/com/cloud/storage/VolumeApiServiceImpl.java 680cd2e
>>
>> Diff: https://reviews.apache.org/r/19616/diff/
>>
>>
>> Testing
>> -------
>>
>> Compiled & ran.
>>
>>
>> Thanks,
>>
>> Alex Hitchins
>
Re: Review Request 19616: Added check for null return.
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19616/#review45234
-----------------------------------------------------------
is this still a valid patch? should it be deleted?
- daan Hoogland
On April 15, 2014, 10:18 a.m., Alex Hitchins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19616/
> -----------------------------------------------------------
>
> (Updated April 15, 2014, 10:18 a.m.)
>
>
> Review request for cloudstack and daan Hoogland.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Added check for returned null, if received then throw exception.
>
> Amendments made as per dev mailing list comments.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/VolumeApiServiceImpl.java 680cd2e
>
> Diff: https://reviews.apache.org/r/19616/diff/
>
>
> Testing
> -------
>
> Compiled & ran.
>
>
> Thanks,
>
> Alex Hitchins
>
>
Re: Review Request 19616: Added check for null return.
Posted by Alex Hitchins <al...@shapeblue.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19616/
-----------------------------------------------------------
(Updated April 15, 2014, 10:18 a.m.)
Review request for cloudstack and daan Hoogland.
Changes
-------
Updated
Repository: cloudstack-git
Description (updated)
-------
Added check for returned null, if received then throw exception.
Amendments made as per dev mailing list comments.
Diffs (updated)
-----
server/src/com/cloud/storage/VolumeApiServiceImpl.java 680cd2e
Diff: https://reviews.apache.org/r/19616/diff/
Testing
-------
Compiled & ran.
Thanks,
Alex Hitchins
RE: Review Request 19616: Added check for null return.
Posted by Alex Hitchins <al...@alexhitchins.com>.
Cool, once I've been over it with a test I'll update the patch.
Alex Hitchins | 07788 423 969 | 01892 523 587
---------------------------------------------
-----Original Message-----
From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
Sent: 11 April 2014 20:56
To: dev
Cc: Alex Hitchins; Edison Su; Laszlo Hornyak
Subject: Re: Review Request 19616: Added check for null return.
sure,
I still don't like the fact that we need to throw an exception that we
caught one level deeper.
@Edison: can we change the internal api to throw the exception?
On Fri, Apr 11, 2014 at 12:47 PM, Alex Hitchins <al...@alexhitchins.com>
wrote:
> OK, so that should be
>
> if(snapshotInfoReturn==null){
> throw new ResourceAllocationException("Take snapshot for
VolumeId:
> "
> + volumeId + " failed.", ResourceType.snapshot);
> }
>
> So I get null - I should raise an exception.
>
> Alex Hitchins | 07788 423 969 | 01892 523 587
> ---------------------------------------------
>
> -----Original Message-----
> From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
> Sent: 11 April 2014 18:33
> To: dev
> Cc: Alex Hitchins; Edison Su; Laszlo Hornyak
> Subject: Re: Review Request 19616: Added check for null return.
>
> On Fri, Apr 11, 2014 at 9:50 AM, Alex Hitchins <al...@alexhitchins.com>
> wrote:
>>
>> if(snapshotInfoReturn.equals(null)){
>> throw new ResourceAllocationException("Take snapshot for
VolumeId:
> "
>> + volumeId + " failed.", ResourceType.snapshot);
>> }
>
>
> This will never throw a ResourceAllocationException. It will throw a
> NullPointerException or pass, as you call a method on an Object to see
> if it is null (no object).
>
> --
> Daan
>
--
Daan
Re: Review Request 19616: Added check for null return.
Posted by Daan Hoogland <da...@gmail.com>.
sure,
I still don't like the fact that we need to throw an exception that we
caught one level deeper.
@Edison: can we change the internal api to throw the exception?
On Fri, Apr 11, 2014 at 12:47 PM, Alex Hitchins <al...@alexhitchins.com> wrote:
> OK, so that should be
>
> if(snapshotInfoReturn==null){
> throw new ResourceAllocationException("Take snapshot for VolumeId:
> "
> + volumeId + " failed.", ResourceType.snapshot);
> }
>
> So I get null - I should raise an exception.
>
> Alex Hitchins | 07788 423 969 | 01892 523 587
> ---------------------------------------------
>
> -----Original Message-----
> From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
> Sent: 11 April 2014 18:33
> To: dev
> Cc: Alex Hitchins; Edison Su; Laszlo Hornyak
> Subject: Re: Review Request 19616: Added check for null return.
>
> On Fri, Apr 11, 2014 at 9:50 AM, Alex Hitchins <al...@alexhitchins.com>
> wrote:
>>
>> if(snapshotInfoReturn.equals(null)){
>> throw new ResourceAllocationException("Take snapshot for VolumeId:
> "
>> + volumeId + " failed.", ResourceType.snapshot);
>> }
>
>
> This will never throw a ResourceAllocationException. It will throw a
> NullPointerException or pass, as you call a method on an Object to see if it
> is null (no object).
>
> --
> Daan
>
--
Daan
RE: Review Request 19616: Added check for null return.
Posted by Alex Hitchins <al...@alexhitchins.com>.
OK, so that should be
if(snapshotInfoReturn==null){
throw new ResourceAllocationException("Take snapshot for VolumeId:
"
+ volumeId + " failed.", ResourceType.snapshot);
}
So I get null - I should raise an exception.
Alex Hitchins | 07788 423 969 | 01892 523 587
---------------------------------------------
-----Original Message-----
From: Daan Hoogland [mailto:daan.hoogland@gmail.com]
Sent: 11 April 2014 18:33
To: dev
Cc: Alex Hitchins; Edison Su; Laszlo Hornyak
Subject: Re: Review Request 19616: Added check for null return.
On Fri, Apr 11, 2014 at 9:50 AM, Alex Hitchins <al...@alexhitchins.com>
wrote:
>
> if(snapshotInfoReturn.equals(null)){
> throw new ResourceAllocationException("Take snapshot for VolumeId:
"
> + volumeId + " failed.", ResourceType.snapshot);
> }
This will never throw a ResourceAllocationException. It will throw a
NullPointerException or pass, as you call a method on an Object to see if it
is null (no object).
--
Daan
Re: Review Request 19616: Added check for null return.
Posted by Daan Hoogland <da...@gmail.com>.
On Fri, Apr 11, 2014 at 9:50 AM, Alex Hitchins <al...@alexhitchins.com> wrote:
>
> if(snapshotInfoReturn.equals(null)){
> throw new ResourceAllocationException("Take snapshot for VolumeId: "
> + volumeId + " failed.", ResourceType.snapshot);
> }
This will never throw a ResourceAllocationException. It will throw a
NullPointerException or pass, as you call a method on an Object to see
if it is null (no object).
--
Daan
RE: Review Request 19616: Added check for null return.
Posted by Alex Hitchins <al...@alexhitchins.com>.
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.hoogland@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
<al...@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
RE: Review Request 19616: Added check for null return.
Posted by Alex Hitchins <al...@alexhitchins.com>.
That's too much common sense for me to cope with.
Thanks for the tip!
-----Original Message-----
From: Daan Hoogland [mailto:daan.hoogland@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 <al...@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.hoogland@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
> <al...@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
Re: Review Request 19616: Added check for null return.
Posted by Daan Hoogland <da...@gmail.com>.
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 <al...@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.hoogland@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 <al...@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
RE: Review Request 19616: Added check for null return.
Posted by Alex Hitchins <al...@alexhitchins.com>.
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.hoogland@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 <al...@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
Re: Review Request 19616: Added check for null return.
Posted by Daan Hoogland <da...@gmail.com>.
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
<al...@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
Re: Review Request 19616: Added check for null return.
Posted by Alex Hitchins <al...@shapeblue.com>.
-----------------------------------------------------------
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
Re: Review Request 19616: Added check for null return.
Posted by Daan Hoogland <da...@gmail.com>.
h Lazlo,
Is this advocacy for proper use of exceptions?
On Tue, Mar 25, 2014 at 11:17 PM, Laszlo Hornyak
<la...@gmail.com> wrote:
> Hi,
>
> I have just looked into this, looks quite safe to me and follows the
> conventions of the code.
>
> However, what about a little change in the conventions? Wouldn't it be more
> simple not to return null if the operation failed?
> This could help simplifying the code by eliminating lots of null-checks,
> and also the service implementations could become more simple.
>
>
>
>
> On Tue, Mar 25, 2014 at 4:04 PM, Alex Hitchins
> <al...@shapeblue.com>wrote:
>
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/19616/
>> -----------------------------------------------------------
>>
>> Review request for cloudstack.
>>
>>
>> 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
>>
>>
>
>
> --
>
> EOF
--
Daan
FW: Review Request 19616: Added check for null return.
Posted by Alex Hitchins <al...@shapeblue.com>.
Hi there,
I did look a that initially, however it meant making a change to the parent method. Not knowing where else this was called from, I didn't want to introduce more bugs elsewhere. I agree It's the best thing to do, however I saw the immediate need to not have the situation where an error occurs and the log says it all went smoothly.
If this patch is committed, it won't break when the parent method does get resolved, it will just be redundant. I hope to get the suggested more encompassing fix done when I can see all the instances where it's used.
I hope my thought process makes sense?
Regards
Alex Hitchins
D: +44 1892 523 587 | S: +44 2036 030 540<tel:+442036030540> | M: +44<tel:+447968161581> 7788 423 969
alex.hitchins@shapeblue.com<ma...@shapeblue.com>
From: Laszlo Hornyak [mailto:laszlo.hornyak@gmail.com]
Sent: 25 March 2014 22:18
To: dev@cloudstack.apache.org<ma...@cloudstack.apache.org>; Alex Hitchins
Subject: Re: Review Request 19616: Added check for null return.
Hi,
I have just looked into this, looks quite safe to me and follows the conventions of the code.
However, what about a little change in the conventions? Wouldn't it be more simple not to return null if the operation failed?
This could help simplifying the code by eliminating lots of null-checks, and also the service implementations could become more simple.
On Tue, Mar 25, 2014 at 4:04 PM, Alex Hitchins <al...@shapeblue.com>> wrote:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19616/
-----------------------------------------------------------
Review request for cloudstack.
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
--
EOF
Need Enterprise Grade Support for Apache CloudStack?
Our CloudStack Infrastructure Support<http://shapeblue.com/cloudstack-infrastructure-support/> offers the best 24/7 SLA for CloudStack Environments.
Apache CloudStack Bootcamp training courses
**NEW!** CloudStack 4.2.1 training<http://shapeblue.com/cloudstack-training/>
18th-19th February 2014, Brazil. Classroom<http://shapeblue.com/cloudstack-training/>
17th-23rd March 2014, Region A. Instructor led, On-line<http://shapeblue.com/cloudstack-training/>
24th-28th March 2014, Region B. Instructor led, On-line<http://shapeblue.com/cloudstack-training/>
16th-20th June 2014, Region A. Instructor led, On-line<http://shapeblue.com/cloudstack-training/>
23rd-27th June 2014, Region B. Instructor led, On-line<http://shapeblue.com/cloudstack-training/>
This email and any attachments to it may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Shape Blue Ltd or related companies. If you are not the intended recipient of this email, you must neither take any action based upon its contents, nor copy or show it to anyone. Please contact the sender if you believe you have received this email in error. Shape Blue Ltd is a company incorporated in England & Wales. ShapeBlue Services India LLP is a company incorporated in India and is operated under license from Shape Blue Ltd. Shape Blue Brasil Consultoria Ltda is a company incorporated in Brasil and is operated under license from Shape Blue Ltd. ShapeBlue is a registered trademark.
Re: Review Request 19616: Added check for null return.
Posted by Laszlo Hornyak <la...@gmail.com>.
Hi,
I have just looked into this, looks quite safe to me and follows the
conventions of the code.
However, what about a little change in the conventions? Wouldn't it be more
simple not to return null if the operation failed?
This could help simplifying the code by eliminating lots of null-checks,
and also the service implementations could become more simple.
On Tue, Mar 25, 2014 at 4:04 PM, Alex Hitchins
<al...@shapeblue.com>wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19616/
> -----------------------------------------------------------
>
> Review request for cloudstack.
>
>
> 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
>
>
--
EOF