You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Alexander Hitchins <al...@shapeblue.com> on 2014/03/20 13:00:59 UTC
Review Request 19460: Check for returned null,
throw exception rather than continue.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19460/
-----------------------------------------------------------
Review request for cloudstack.
Repository: cloudstack-git
Description
-------
https://issues.apache.org/jira/browse/CLOUDSTACK-5825
I've added a check for null returned. Previously it would return even though there were errors. If null is returned, the takeSnapshot raised and caught an exception. I check for null and if received throw a new exception rather than continue the flow.
Diffs
-----
server/src/com/cloud/storage/VolumeApiServiceImpl.java 5ffa99b
Diff: https://reviews.apache.org/r/19460/diff/
Testing
-------
Compiled and deployed on a new test system, completed steps and snapshots created successfully.
I could not re-create a snapshot error, however normal operation is as expected. It's better it throws an exception rather than continue to log it's successful when it's not.
Thanks,
Alexander Hitchins
Re: Review Request 19460: Check for returned null,
throw exception rather than continue.
Posted by Alex Hitchins <al...@shapeblue.com>.
> On March 20, 2014, 1:29 p.m., Chris Suich wrote:
> > It looks like VolumeServiceImpl.takeSnapshot() is the method who isn't properly handling the exception. It is also called from multiple places (namely VolumeApiServiceImpl.takeSnapshot() and VolumeApiServiceImpl.orchestrateTakeVolumeSnapshot()).
> >
> > It looks like your fix is only handling one of those cases. If you were to put the throw statement into VolumeServiceImpl.takeSnapshot()'s catch instead of just ignoring the exception it would be handled for anyone who tries to take a snapshot.
Good point - I shall move the throw inside the takeSnapshot method.
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19460/#review37881
-----------------------------------------------------------
On March 20, 2014, noon, Alex Hitchins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19460/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, noon)
>
>
> Review request for cloudstack.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/CLOUDSTACK-5825
>
> I've added a check for null returned. Previously it would return even though there were errors. If null is returned, the takeSnapshot raised and caught an exception. I check for null and if received throw a new exception rather than continue the flow.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/VolumeApiServiceImpl.java 5ffa99b
>
> Diff: https://reviews.apache.org/r/19460/diff/
>
>
> Testing
> -------
>
> Compiled and deployed on a new test system, completed steps and snapshots created successfully.
>
> I could not re-create a snapshot error, however normal operation is as expected. It's better it throws an exception rather than continue to log it's successful when it's not.
>
>
> Thanks,
>
> Alex Hitchins
>
>
Re: Review Request 19460: Check for returned null,
throw exception rather than continue.
Posted by Chris Suich <ch...@netapp.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19460/#review37881
-----------------------------------------------------------
It looks like VolumeServiceImpl.takeSnapshot() is the method who isn't properly handling the exception. It is also called from multiple places (namely VolumeApiServiceImpl.takeSnapshot() and VolumeApiServiceImpl.orchestrateTakeVolumeSnapshot()).
It looks like your fix is only handling one of those cases. If you were to put the throw statement into VolumeServiceImpl.takeSnapshot()'s catch instead of just ignoring the exception it would be handled for anyone who tries to take a snapshot.
- Chris Suich
On March 20, 2014, noon, Alex Hitchins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19460/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, noon)
>
>
> Review request for cloudstack.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/CLOUDSTACK-5825
>
> I've added a check for null returned. Previously it would return even though there were errors. If null is returned, the takeSnapshot raised and caught an exception. I check for null and if received throw a new exception rather than continue the flow.
>
>
> Diffs
> -----
>
> server/src/com/cloud/storage/VolumeApiServiceImpl.java 5ffa99b
>
> Diff: https://reviews.apache.org/r/19460/diff/
>
>
> Testing
> -------
>
> Compiled and deployed on a new test system, completed steps and snapshots created successfully.
>
> I could not re-create a snapshot error, however normal operation is as expected. It's better it throws an exception rather than continue to log it's successful when it's not.
>
>
> Thanks,
>
> Alex Hitchins
>
>
Re: Review Request 19460: Check for returned null,
throw exception rather than continue.
Posted by Chris Suich <ch...@netapp.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19460/#review37891
-----------------------------------------------------------
Ship it!
Ship It!
- Chris Suich
On March 20, 2014, 2:42 p.m., Alex Hitchins wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19460/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 2:42 p.m.)
>
>
> Review request for cloudstack.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/CLOUDSTACK-5825
>
> I've added a check for null returned. Previously it would return even though there were errors. If null is returned, the takeSnapshot raised and caught an exception. I check for null and if received throw a new exception rather than continue the flow.
>
>
> Diffs
> -----
>
> engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java 68e5a56
>
> Diff: https://reviews.apache.org/r/19460/diff/
>
>
> Testing
> -------
>
> Compiled and deployed on a new test system, completed steps and snapshots created successfully.
>
> I could not re-create a snapshot error, however normal operation is as expected. It's better it throws an exception rather than continue to log it's successful when it's not.
>
>
> Thanks,
>
> Alex Hitchins
>
>
Re: Review Request 19460: Check for returned null,
throw exception rather than continue.
Posted by Alex Hitchins <al...@shapeblue.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19460/
-----------------------------------------------------------
(Updated March 20, 2014, 2:42 p.m.)
Review request for cloudstack.
Changes
-------
Amended patch to throw exception inside takeSnapshot method.
Repository: cloudstack-git
Description
-------
https://issues.apache.org/jira/browse/CLOUDSTACK-5825
I've added a check for null returned. Previously it would return even though there were errors. If null is returned, the takeSnapshot raised and caught an exception. I check for null and if received throw a new exception rather than continue the flow.
Diffs (updated)
-----
engine/storage/volume/src/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java 68e5a56
Diff: https://reviews.apache.org/r/19460/diff/
Testing
-------
Compiled and deployed on a new test system, completed steps and snapshots created successfully.
I could not re-create a snapshot error, however normal operation is as expected. It's better it throws an exception rather than continue to log it's successful when it's not.
Thanks,
Alex Hitchins