You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Darren Shepherd <da...@gmail.com> on 2013/10/22 05:34:25 UTC
Review Request 14823: Refactor StrategyPriority
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14823/
-----------------------------------------------------------
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
Re: Review Request 14823: Refactor StrategyPriority
Posted by Darren Shepherd <da...@gmail.com>.
> 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
>
>
Re: Review Request 14823: Refactor StrategyPriority
Posted by Chris Suich <ch...@netapp.com>.
> 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.
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)?
- Chris
-----------------------------------------------------------
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
>
>
Re: Review Request 14823: Refactor StrategyPriority
Posted by Chris Suich <ch...@netapp.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14823/#review27296
-----------------------------------------------------------
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
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
>
>