> On Oct. 22, 2013, 1:19 p.m., Chris Suich wrote: > > Darren, the design of all of this has already changed here: > > https://reviews.apache.org/r/14522/. Take a look at the newer revisions > > which haven't made it in yet (still waiting on a final review from John > > Burwell). Could you take a look there and see if those changes address your > > issue? In this new approach, we no longer create a sorted list. Instead, we > > simply chose one by iterating over the list of strategies once and > > returning the best strategy. > > Chris Suich wrote: > That being said, your changes appear to make things cleaner. Maybe if > mine are accepted we can rebase yours on top of mine (i.e. replace sort() > with the new approach)?
That sounds good. I was just thinking that it seems like we need a factory or locator pattern. It seems silly to inject all the strategies to a calling class just so it can delegate to something else to pick the right strategy from the list it has. So centralizing everything to factory seems to be a little cleaner approach. Also all the comparators just seemed like a lot of boilerplate code. - Darren ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14823/#review27296 ----------------------------------------------------------- On Oct. 22, 2013, 3:34 a.m., Darren Shepherd wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14823/ > ----------------------------------------------------------- > > (Updated Oct. 22, 2013, 3:34 a.m.) > > > Review request for cloudstack, Chris Suich and edison su. > > > Repository: cloudstack-git > > > Description > ------- > > I specifically ran into a problem in the spring modularization branch with > StrategyPriority.sortStrategies(). The lists of strategies are unmodifiable > collections so Collection.sort() can not be used. I went to do a quick fix > for this but then decided to try remove the duplicated logic in each code > that needs a SnapshotStrategy or DataMotionStrategy and then ended up > refactoring quite a bit. > > > Diffs > ----- > > > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/DataMotionStrategy.java > 950f9e2 > > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/SnapshotStrategy.java > e4cecb6 > > engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StorageStrategyFactory.java > PRE-CREATION > > 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/AncientDataMotionStrategy.java > 5f5f01e > > engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/DataMotionServiceImpl.java > 2d31320 > > engine/storage/snapshot/src/org/apache/cloudstack/storage/snapshot/XenserverSnapshotStrategy.java > 6a874d6 > > engine/storage/src/org/apache/cloudstack/storage/helper/StorageStrategyFactoryImpl.java > PRE-CREATION > > engine/storage/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java > PRE-CREATION > > plugins/hypervisors/xen/src/org/apache/cloudstack/storage/motion/XenServerStorageMotionStrategy.java > 8578a9a > server/src/com/cloud/storage/snapshot/SnapshotManagerImpl.java dade983 > > Diff: https://reviews.apache.org/r/14823/diff/ > > > Testing > ------- > > > Thanks, > > Darren Shepherd > >
