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,

Re: Review Request 19616: Added check for null return.

h Lazlo, Is this advocacy for proper use of exceptions? On Tue, Mar 25, 2014 at 11:17 PM, Laszlo Hornyak 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 mor

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/ --- (Updated April 3, 2014, 12:24 p.m.) Review request for cloudstack. Changes --

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 takeSna

RE: Review Request 19616: Added check for null return.

;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 quic

Re: Review Request 19616: Added check for null return.

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 retu

RE: Review Request 19616: Added check for null return.

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 r

RE: Review Request 19616: Added check for null return.

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 Ale

Re: Review Request 19616: Added check for null return.

On Fri, Apr 11, 2014 at 9:50 AM, Alex Hitchins wrote: > > if(snapshotInfoReturn.equals(null)){ > throw new ResourceAllocationException("Take snapshot for VolumeId: " > + volumeId + " failed.", ResourceType.snapshot); > } This will never throw a ResourceAllocationException. It

RE: Review Request 19616: Added check for null return.

969 | 01892 523 587 - -Original Message- From: Daan Hoogland [mailto:daan.hoogl...@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

Re: Review Request 19616: Added check for null return.

n. > > Alex Hitchins | 07788 423 969 | 01892 523 587 > - > > -Original Message- > From: Daan Hoogland [mailto:daan.hoogl...@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 r

RE: Review Request 19616: Added check for null return.

x 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

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/ --- (Updated April 15, 2014, 10:18 a.m.) Review request for cloudstack and daan Hoo

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

RE: Review Request 19616: Added check for null return.

gland 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/

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/#review45234 --- is this still a valid patch? should it be deleted? - daan Hoogland

Re: Review Request 19616: Added check for null return.

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" wrote: > > > --- > This is an automatically generated e-mail. To rep

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 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" wrote: >> >> >> --

RE: Review Request 19616: Added check for null return.

I will take a look later tonight. Alex Hitchins | 07788 423 969 | 01892 523 587 -Original Message- From: Daan Hoogland [mailto:daan.hoogl...@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 upl

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/ --- (Updated July 1, 2014, 3:29 p.m.) Review request for cloudstack. Changes

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/#review79042 --- Thank you for submitting your CloudStack contribution through review