----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14522/#review27021 -----------------------------------------------------------
api/src/org/apache/cloudstack/api/response/SnapshotResponse.java <https://reviews.apache.org/r/14522/#comment52591> CanRevert is an awkward/non-standard property name. Prefer an adverb such as revertable (e.g. isRevertable/setRevertable with an attribute named revertableFlag). engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreDriver.java <https://reviews.apache.org/r/14522/#comment52592> I don't know that a flag is necessary to represent this capability since we already have a revertSnapshot method. Personally, I dislike the complexity of exposing methods on an interface that are guarded by another. I would prefer to see revertSnapshot enhanced to throw an exception when a snapshot can't be reverted. In addition to be a simpler approach, it covers both the scenario when a device does not support reverting snapshots and, as well as, the device or the snapshot not being a revertable state. engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotInfo.java <https://reviews.apache.org/r/14522/#comment52593> Prefer an adverb property name such as "revertable". engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java <https://reviews.apache.org/r/14522/#comment52595> How does the result of the canHandle method drive the comparison of two Snapshots? Why not just compare the ops to each other other? engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java <https://reviews.apache.org/r/14522/#comment52594> Very poor performance as it circumvents the primitive pool. Change to Integer.valueOf(i1).compareTo(Integer.valueOf(i2)). engine/storage/integration-test/test/org/apache/cloudstack/storage/test/SnapshotTest.java <https://reviews.apache.org/r/14522/#comment52596> *This comment may be more editorial at this point because of the nature of the existing code* I would prefer to see an exception thrown here rather than the complexity of the canHandle. I am also concerned that the canHandle method only checks that the underlying device can take a snapshot without accounting for whether or not the volume is in a state to take a consistent snapshot. I am also concerned that we don't have a semantic for handling errors that might result from the actual snapshot operation. engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/SnapshotObject.java <https://reviews.apache.org/r/14522/#comment52597> What is the purpose of sorting the strategies? engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java <https://reviews.apache.org/r/14522/#comment52598> Coding convention: Add curly braces server/src/com/cloud/api/ApiResponseHelper.java <https://reviews.apache.org/r/14522/#comment52599> If the snapshot was instance of SnapshotInfo and no SnapshotInfo was found, an exception should thrown. Provided my assumption is correct, I suggest refactoring to a structure such as the following: final SnapshotInfo secondaryInfo = (snapshot instanceof SnapshotInfo) ? (SnapshotInfo) snapshot : snapshotfactory.getSnapshot(snapshot.getId(), DataStoreRole.Image); if (secondaryInfo == null) { throw new CloudRuntimException(/* Appropriate error message */ ); } snapshotResponse.setCanRevert(secondaryInfo.canRevert); - John Burwell On Oct. 14, 2013, 6:50 p.m., Chris Suich wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14522/ > ----------------------------------------------------------- > > (Updated Oct. 14, 2013, 6: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 f85784b > 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/SnapshotStrategy.java > e4cecb6 > > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java > 8bd04f5 > > engine/api/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java > 329b99f > > 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 > >
