> 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. > > Chris Suich wrote: > 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. > > John Burwell wrote: > Enough details so an operator/admin could look up the associated > volume/snapshot and debug the issue. Likely a good start would be to include > the identifying information of the srcData and destData parameters. Ask > yourself, "What I need to know as an operator/administrator to debug this > problem?" and put that information in the error message.
> 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. > > Chris Suich wrote: > 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. > > John Burwell wrote: > I missed the overload. The actual issue, in my mind, is that > pickStrategy needs to be DRY'ed out. This likely requires a new interface > which is outside the bounds of this patch. However, it is technical debt > that we somehow need to surface. As such, please open a ticket to describing > issue and put a TODO on pickStrategy to consolidate the implementations with > a reference to the ticket. Also, place a TODO on this lines to reference the > future refactoring. - Chris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14522/#review27126 ----------------------------------------------------------- On Oct. 17, 2013, 6:46 p.m., Chris Suich wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14522/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2013, 6:46 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 > ----- > > > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriority.java > 81034b1 > > engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java > 2d31320 > server/src/com/cloud/api/ApiResponseHelper.java f4ca112 > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java dade983 > > 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 > >
