Return-Path: X-Original-To: apmail-cloudstack-commits-archive@www.apache.org Delivered-To: apmail-cloudstack-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id D8DE410889 for ; Wed, 23 Oct 2013 23:37:05 +0000 (UTC) Received: (qmail 11823 invoked by uid 500); 23 Oct 2013 23:37:05 -0000 Delivered-To: apmail-cloudstack-commits-archive@cloudstack.apache.org Received: (qmail 11804 invoked by uid 500); 23 Oct 2013 23:37:05 -0000 Mailing-List: contact commits-help@cloudstack.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@cloudstack.apache.org Delivered-To: mailing list commits@cloudstack.apache.org Received: (qmail 11797 invoked by uid 99); 23 Oct 2013 23:37:05 -0000 Received: from tyr.zones.apache.org (HELO tyr.zones.apache.org) (140.211.11.114) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 23 Oct 2013 23:37:05 +0000 Received: by tyr.zones.apache.org (Postfix, from userid 65534) id 522A58191BA; Wed, 23 Oct 2013 23:37:05 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: darren@apache.org To: commits@cloudstack.apache.org Message-Id: <0bada820c9aa4de692732a491a96f1ba@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: git commit: updated refs/heads/master to f3e968b Date: Wed, 23 Oct 2013 23:37:05 +0000 (UTC) Updated Branches: refs/heads/master b74754959 -> f3e968b98 Update StorageStrategyFactory to not sort, but just iterate and find the first Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/f3e968b9 Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/f3e968b9 Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/f3e968b9 Branch: refs/heads/master Commit: f3e968b9830a667cb3b55477f1fe8259e56ceffb Parents: b747549 Author: Darren Shepherd Authored: Wed Oct 23 16:36:31 2013 -0700 Committer: Darren Shepherd Committed: Wed Oct 23 16:36:31 2013 -0700 ---------------------------------------------------------------------- .../api/storage/StorageStrategyFactory.java | 9 -- .../helper/StorageStrategyFactoryImpl.java | 57 ++++-------- .../api/storage/StrategyPriorityTest.java | 93 ++++++++++++++------ 3 files changed, 82 insertions(+), 77 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cloudstack/blob/f3e968b9/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StorageStrategyFactory.java ---------------------------------------------------------------------- diff --git a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StorageStrategyFactory.java b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StorageStrategyFactory.java index 3a73cd0..ac1e311 100644 --- a/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StorageStrategyFactory.java +++ b/engine/api/src/org/apache/cloudstack/engine/subsystem/api/storage/StorageStrategyFactory.java @@ -18,7 +18,6 @@ */ package org.apache.cloudstack.engine.subsystem.api.storage; -import java.util.Collection; import java.util.Map; import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy.SnapshotOperation; @@ -28,18 +27,10 @@ import com.cloud.storage.Snapshot; public interface StorageStrategyFactory { - Collection getDataMotionStrategies(DataObject srcData, DataObject destData); - DataMotionStrategy getDataMotionStrategy(DataObject srcData, DataObject destData); - - Collection getDataMotionStrategies(Map volumeMap, Host srcHost, Host destHost); - DataMotionStrategy getDataMotionStrategy(Map volumeMap, Host srcHost, Host destHost); - - Collection getSnapshotStrategies(Snapshot snapshot, SnapshotOperation op); - SnapshotStrategy getSnapshotStrategy(Snapshot snapshot, SnapshotOperation op); } http://git-wip-us.apache.org/repos/asf/cloudstack/blob/f3e968b9/engine/storage/src/org/apache/cloudstack/storage/helper/StorageStrategyFactoryImpl.java ---------------------------------------------------------------------- diff --git a/engine/storage/src/org/apache/cloudstack/storage/helper/StorageStrategyFactoryImpl.java b/engine/storage/src/org/apache/cloudstack/storage/helper/StorageStrategyFactoryImpl.java index 0bbd28b..30812bf 100644 --- a/engine/storage/src/org/apache/cloudstack/storage/helper/StorageStrategyFactoryImpl.java +++ b/engine/storage/src/org/apache/cloudstack/storage/helper/StorageStrategyFactoryImpl.java @@ -19,10 +19,8 @@ package org.apache.cloudstack.storage.helper; import java.util.Collection; -import java.util.Comparator; import java.util.List; import java.util.Map; -import java.util.TreeSet; import javax.inject.Inject; @@ -30,10 +28,10 @@ import org.apache.cloudstack.engine.subsystem.api.storage.DataMotionStrategy; import org.apache.cloudstack.engine.subsystem.api.storage.DataObject; import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy; +import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy.SnapshotOperation; import org.apache.cloudstack.engine.subsystem.api.storage.StorageStrategyFactory; import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority; import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo; -import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy.SnapshotOperation; import com.cloud.host.Host; import com.cloud.storage.Snapshot; @@ -44,23 +42,8 @@ public class StorageStrategyFactoryImpl implements StorageStrategyFactory { List dataMotionStrategies; @Override - public DataMotionStrategy getDataMotionStrategy(DataObject srcData, DataObject destData) { - return first(getDataMotionStrategies(srcData, destData)); - } - - @Override - public DataMotionStrategy getDataMotionStrategy(Map volumeMap, Host srcHost, Host destHost) { - return first(getDataMotionStrategies(volumeMap, srcHost, destHost)); - } - - @Override - public SnapshotStrategy getSnapshotStrategy(Snapshot snapshot, SnapshotOperation op) { - return first(getSnapshotStrategies(snapshot, op)); - } - - @Override - public Collection getDataMotionStrategies(final DataObject srcData, final DataObject destData) { - return sort(dataMotionStrategies, new CanHandle() { + public DataMotionStrategy getDataMotionStrategy(final DataObject srcData, final DataObject destData) { + return bestMatch(dataMotionStrategies, new CanHandle() { @Override public StrategyPriority canHandle(DataMotionStrategy strategy) { return strategy.canHandle(srcData, destData); @@ -69,8 +52,8 @@ public class StorageStrategyFactoryImpl implements StorageStrategyFactory { } @Override - public Collection getDataMotionStrategies(final Map volumeMap, final Host srcHost, final Host destHost) { - return sort(dataMotionStrategies, new CanHandle() { + public DataMotionStrategy getDataMotionStrategy(final Map volumeMap, final Host srcHost, final Host destHost) { + return bestMatch(dataMotionStrategies, new CanHandle() { @Override public StrategyPriority canHandle(DataMotionStrategy strategy) { return strategy.canHandle(volumeMap, srcHost, destHost); @@ -79,8 +62,8 @@ public class StorageStrategyFactoryImpl implements StorageStrategyFactory { } @Override - public Collection getSnapshotStrategies(final Snapshot snapshot, final SnapshotOperation op) { - return sort(snapshotStrategies, new CanHandle() { + public SnapshotStrategy getSnapshotStrategy(final Snapshot snapshot, final SnapshotOperation op) { + return bestMatch(snapshotStrategies, new CanHandle() { @Override public StrategyPriority canHandle(SnapshotStrategy strategy) { return strategy.canHandle(snapshot, op); @@ -88,30 +71,22 @@ public class StorageStrategyFactoryImpl implements StorageStrategyFactory { }); } - private static Collection sort(Collection collection, final CanHandle canHandle) { + private static T bestMatch(Collection collection, final CanHandle canHandle) { if (collection.size() == 0) return null; - TreeSet resultSet = new TreeSet(new Comparator() { - @Override - public int compare(T o1, T o2) { - int i1 = canHandle.canHandle(o1).ordinal(); - int i2 = canHandle.canHandle(o2).ordinal(); - return new Integer(i2).compareTo(new Integer(i1)); - } - }); + StrategyPriority highestPriority = StrategyPriority.CANT_HANDLE; - for ( T test : collection ) { - if ( canHandle.canHandle(test) != StrategyPriority.CANT_HANDLE ) { - resultSet.add(test); + T strategyToUse = null; + for (T strategy : collection) { + StrategyPriority priority = canHandle.canHandle(strategy); + if (priority.ordinal() > highestPriority.ordinal()) { + highestPriority = priority; + strategyToUse = strategy; } } - return resultSet; - } - - private static T first(Collection resultSet) { - return resultSet.size() == 0 ? null : resultSet.iterator().next(); + return strategyToUse; } private static interface CanHandle { http://git-wip-us.apache.org/repos/asf/cloudstack/blob/f3e968b9/engine/storage/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java ---------------------------------------------------------------------- diff --git a/engine/storage/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java b/engine/storage/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java index 8cfa955..1c3ceb6 100644 --- a/engine/storage/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java +++ b/engine/storage/test/org/apache/cloudstack/engine/subsystem/api/storage/StrategyPriorityTest.java @@ -16,23 +16,23 @@ // under the License. package org.apache.cloudstack.engine.subsystem.api.storage; -import static org.junit.Assert.*; -import static org.mockito.Matchers.*; -import static org.mockito.Mockito.*; - import java.util.ArrayList; -import java.util.Arrays; -import java.util.Iterator; import java.util.List; import java.util.Map; import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotStrategy.SnapshotOperation; +import org.apache.cloudstack.engine.subsystem.api.storage.StrategyPriority; import org.apache.cloudstack.storage.helper.StorageStrategyFactoryImpl; import org.junit.Test; import com.cloud.host.Host; import com.cloud.storage.Snapshot; +import static org.junit.Assert.assertEquals; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; + public class StrategyPriorityTest { @Test @@ -50,17 +50,30 @@ public class StrategyPriorityTest { doReturn(StrategyPriority.HIGHEST).when(highestStrategy).canHandle(any(Snapshot.class), any(SnapshotOperation.class)); List strategies = new ArrayList(5); - strategies.addAll(Arrays.asList(defaultStrategy, pluginStrategy, hyperStrategy, cantHandleStrategy, highestStrategy)); + SnapshotStrategy strategy = null; StorageStrategyFactoryImpl factory = new StorageStrategyFactoryImpl(); factory.setSnapshotStrategies(strategies); - Iterator iter = factory.getSnapshotStrategies(mock(Snapshot.class), SnapshotOperation.TAKE).iterator(); - assertEquals("Highest was not 1st.", highestStrategy, iter.next()); - assertEquals("Plugin was not 2nd.", pluginStrategy, iter.next()); - assertEquals("Hypervisor was not 3rd.", hyperStrategy, iter.next()); - assertEquals("Default was not 4th.", defaultStrategy, iter.next()); - assertTrue("Can't Handle was not 5th.", !iter.hasNext()); + strategies.add(cantHandleStrategy); + strategy = factory.getSnapshotStrategy(mock(Snapshot.class), SnapshotOperation.TAKE); + assertEquals("A strategy was found when it shouldn't have been.", null, strategy); + + strategies.add(defaultStrategy); + strategy = factory.getSnapshotStrategy(mock(Snapshot.class), SnapshotOperation.TAKE); + assertEquals("Default strategy was not picked.", defaultStrategy, strategy); + + strategies.add(hyperStrategy); + strategy = factory.getSnapshotStrategy(mock(Snapshot.class), SnapshotOperation.TAKE); + assertEquals("Hypervisor strategy was not picked.", hyperStrategy, strategy); + + strategies.add(pluginStrategy); + strategy = factory.getSnapshotStrategy(mock(Snapshot.class), SnapshotOperation.TAKE); + assertEquals("Plugin strategy was not picked.", pluginStrategy, strategy); + + strategies.add(highestStrategy); + strategy = factory.getSnapshotStrategy(mock(Snapshot.class), SnapshotOperation.TAKE); + assertEquals("Highest strategy was not picked.", highestStrategy, strategy); } @Test @@ -78,17 +91,30 @@ public class StrategyPriorityTest { doReturn(StrategyPriority.HIGHEST).when(highestStrategy).canHandle(any(DataObject.class), any(DataObject.class)); List strategies = new ArrayList(5); - strategies.addAll(Arrays.asList(defaultStrategy, pluginStrategy, hyperStrategy, cantHandleStrategy, highestStrategy)); + DataMotionStrategy strategy = null; StorageStrategyFactoryImpl factory = new StorageStrategyFactoryImpl(); factory.setDataMotionStrategies(strategies); - Iterator iter = factory.getDataMotionStrategies(mock(DataObject.class), mock(DataObject.class)).iterator(); - assertEquals("Highest was not 1st.", highestStrategy, iter.next()); - assertEquals("Plugin was not 2nd.", pluginStrategy, iter.next()); - assertEquals("Hypervisor was not 3rd.", hyperStrategy, iter.next()); - assertEquals("Default was not 4th.", defaultStrategy, iter.next()); - assertTrue("Can't Handle was not 5th.", !iter.hasNext()); + strategies.add(cantHandleStrategy); + strategy = factory.getDataMotionStrategy(mock(DataObject.class), mock(DataObject.class)); + assertEquals("A strategy was found when it shouldn't have been.", null, strategy); + + strategies.add(defaultStrategy); + strategy = factory.getDataMotionStrategy(mock(DataObject.class), mock(DataObject.class)); + assertEquals("Default strategy was not picked.", defaultStrategy, strategy); + + strategies.add(hyperStrategy); + strategy = factory.getDataMotionStrategy(mock(DataObject.class), mock(DataObject.class)); + assertEquals("Hypervisor strategy was not picked.", hyperStrategy, strategy); + + strategies.add(pluginStrategy); + strategy = factory.getDataMotionStrategy(mock(DataObject.class), mock(DataObject.class)); + assertEquals("Plugin strategy was not picked.", pluginStrategy, strategy); + + strategies.add(highestStrategy); + strategy = factory.getDataMotionStrategy(mock(DataObject.class), mock(DataObject.class)); + assertEquals("Highest strategy was not picked.", highestStrategy, strategy); } @Test @@ -107,16 +133,29 @@ public class StrategyPriorityTest { doReturn(StrategyPriority.HIGHEST).when(highestStrategy).canHandle(any(Map.class), any(Host.class), any(Host.class)); List strategies = new ArrayList(5); - strategies.addAll(Arrays.asList(defaultStrategy, pluginStrategy, hyperStrategy, cantHandleStrategy, highestStrategy)); + DataMotionStrategy strategy = null; StorageStrategyFactoryImpl factory = new StorageStrategyFactoryImpl(); factory.setDataMotionStrategies(strategies); - Iterator iter = factory.getDataMotionStrategies(mock(Map.class), mock(Host.class), mock(Host.class)).iterator(); - assertEquals("Highest was not 1st.", highestStrategy, iter.next()); - assertEquals("Plugin was not 2nd.", pluginStrategy, iter.next()); - assertEquals("Hypervisor was not 3rd.", hyperStrategy, iter.next()); - assertEquals("Default was not 4th.", defaultStrategy, iter.next()); - assertTrue("Can't Handle was not 5th.", !iter.hasNext()); + strategies.add(cantHandleStrategy); + strategy = factory.getDataMotionStrategy(mock(Map.class), mock(Host.class), mock(Host.class)); + assertEquals("A strategy was found when it shouldn't have been.", null, strategy); + + strategies.add(defaultStrategy); + strategy = factory.getDataMotionStrategy(mock(Map.class), mock(Host.class), mock(Host.class)); + assertEquals("Default strategy was not picked.", defaultStrategy, strategy); + + strategies.add(hyperStrategy); + strategy = factory.getDataMotionStrategy(mock(Map.class), mock(Host.class), mock(Host.class)); + assertEquals("Hypervisor strategy was not picked.", hyperStrategy, strategy); + + strategies.add(pluginStrategy); + strategy = factory.getDataMotionStrategy(mock(Map.class), mock(Host.class), mock(Host.class)); + assertEquals("Plugin strategy was not picked.", pluginStrategy, strategy); + + strategies.add(highestStrategy); + strategy = factory.getDataMotionStrategy(mock(Map.class), mock(Host.class), mock(Host.class)); + assertEquals("Highest strategy was not picked.", highestStrategy, strategy); } }