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