> On Oct. 17, 2013, 3:18 p.m., John Burwell wrote: > > engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java, > > line 62 > > <https://reviews.apache.org/r/14522/diff/3/?file=365094#file365094line62> > > > > Add context information to error message to help operators and users > > determine what couldn't be snapshotted.
Fair enough - I just copied these existing error messages. What kind of context would you like to see in this error message? The src & dest datastores, files, etc.? I agree that there needs to be context, but this would potentially be a user facing error, so I also don't want to overload them with context that means nothing to them. > On Oct. 17, 2013, 3:18 p.m., John Burwell wrote: > > engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java, > > line 79 > > <https://reviews.apache.org/r/14522/diff/3/?file=365094#file365094line79> > > > > DRY out this code (e.g. extract to a private method). This rules needs > > to be consolidated + add the context information mentioned above. The two invocations of pickStrategy() are actually calling an overloaded function. I'm not sure how we could DRY this out since they method calls/parameter lists are actually different. > On Oct. 17, 2013, 3:18 p.m., John Burwell wrote: > > ui/scripts/storage.js, line 1963 > > <https://reviews.apache.org/r/14522/diff/3/?file=365104#file365104line1963> > > > > How will a corrupted snapshot failure be surfaced to the user? It depends on what you mean by corrupted. If we can no longer find the snapshot we expect from the DB, then the API will throw an exception, per your other review, and the list of snapshots will never populate. If the file exists but is corrupted, then that wouldn't be discovered until either the attempt to revert the snapshot or after the revert operation and the user attempts to use the volume. - Chris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14522/#review27126 ----------------------------------------------------------- On Oct. 16, 2013, 2:50 p.m., Chris Suich wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14522/ > ----------------------------------------------------------- > > (Updated Oct. 16, 2013, 2:50 p.m.) > > > Review request for cloudstack, Brian Federle, edison su, John Burwell, and > Mike Tutkowski. > > > Repository: cloudstack-git > > > Description > ------- > > After the last batch of work to the revertSnapshot API, SnapshotServiceImpl > was not tied into the workflow to be used by storage providers. I have added > the logic in a similar fashion to takeSnapshot(), backupSnapshot() and > deleteSnapshot(). > > I have also added a 'Revert to Snapshot' action to the volume snapshots list > in the UI. > > > Diffs > ----- > > api/src/org/apache/cloudstack/api/ApiConstants.java 62eed09 > api/src/org/apache/cloudstack/api/response/SnapshotResponse.java e9cb109 > > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreDriver.java > b124d83 > > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java > 8d6b760 > > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java > 81034b1 > > engine/api/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java > 3d75279 > > engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java > 2d31320 > > engine/storage/integration-test/test/org/apache/cloudstack/storage/test/FakePrimaryDataStoreDriver.java > 810afd1 > > engine/storage/integration-test/test/org/apache/cloudstack/storage/test/SnapshotTest.java > 81f77d6 > > engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java > 3f35e1d > > engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java > 6a874d6 > > plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/driver/CloudStackPrimaryDataStoreDriverImpl.java > 3eaeb1f > > plugins/storage/volume/sample/src/org/apache/cloudstack/storage/datastore/driver/SamplePrimaryDataStoreDriverImpl.java > ece7b26 > > plugins/storage/volume/solidfire/src/org/apache/cloudstack/storage/datastore/driver/SolidfirePrimaryDataStoreDriver.java > 8046b6c > server/src/com/cloud/api/ApiResponseHelper.java f4ca112 > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java dade983 > ui/scripts/storage.js b16f4d4 > > Diff: https://reviews.apache.org/r/14522/diff/ > > > Testing > ------- > > I have tested all of this locally with a custom storage provider. > > Unfortunately, I'm still in the middle of figuring out how to properly unit > test this type of code. If anyone has any recommendations, please let me know. > > > Thanks, > > Chris Suich > >
