[FIXED JENKINS-17469] Clearer separation of concerns

Refactoring classses to make the concerns of each class clearer. All state changes of an ExternalResource are now made by the ExternalResourceManager instead of whatever object is requesting the resource. To make this happen, the reserve/lock/release methods have been re-written using the Template Method Pattern, implementing the state change flow in the abstract base class and letting sub-classes continue to implement the inner locking mechanism by calling the abstract method reservation/locking/releasingMechanism(). A sequence diagram of the new plugin flow can be seen here: http://www.websequencediagrams.com/cgi-bin/cdraw?lz=cGFydGljaXBhbnQgRXh0ZXJuYWxSZXNRdWV1ZVRhc2tEaXNwYXRjaGVyCgAfDFNlbGVjdGlvbkNyaXRlcmlhABENUmVsZWFzZVJ1bkxpc3RlbgAuDwBdC291cmNlTWFuYWcAUg9BdmFpbGFiaWxpdHlGaWx0ACQfCgpvcHQgV2hlbiBsb29raW5nIGZvciBleGVjdXRvcgoAgT0eLT4AXRM6IGdldACBGxBzTGlzdChub2RlKQAeNmYAgUcFRW5hYmxlZGFuZACBYQdsZSgAGjdnZXRNYXRjaGluZwCBHAkAKCQAgm4XOiByZXNlcnZlKHIAgxcHAIFcDQCDJAwAMhM6IHNldFIAOwZkKCkKZW5kAIMPC2pvYiBzdGFydHMKAIQvESAtLT4AexgAgQoIKSAgW2lmIG5vdACBHQhkXSAAgQYYIACBURMgOgCBEQ4AcxMAgX4bbG9jawCBXDpMb2NrAIF9F2ZpbmlzaGVzCgCGJhIAgXsaIHIAhl8GKCkAgWoZAIJAEgB-DG51bGwAgxwG&s=default
jenkinsci · Apr 10, 2013 · b87513b · b87513b
1 parent d2e2076
commit b87513b
Show file tree

Hide file tree

Showing 10 changed files with 95 additions and 26 deletions.
diff --git a/...sson/jenkins/plugins/externalresource/dispatcher/ExternalResourceQueueTaskDispatcher.java b/...sson/jenkins/plugins/externalresource/dispatcher/ExternalResourceQueueTaskDispatcher.java
@@ -113,14 +113,12 @@ public CauseOfBlockage canTake(Node node, Queue.BuildableItem item) {
 
         for (ExternalResource resource : resources) {
             StashResult result = manager.reserve(node, resource, PluginImpl.getInstance().getReserveTime(),
-                    Jenkins.getInstance().getRootUrl() + item.task.getUrl());
+                    item.task.getUrl());
             logger.log(Level.FINEST, "Reserve result for [{0}]: Status {1} code {2} message {3}",
                     new Object[]{resource.getFullName(), result.getStatus().name(),
                             result.getErrorCode(), result.getMessage(), });
             if (result != null && result.isOk()) {
                 reservedResource = resource;
-                StashInfo info = new StashInfo(result, getUrl(item.task));
-                reservedResource.setReserved(info);
                 logger.finest("reservation ok");
                 break;
             } else {

diff --git a/...java/com/sonyericsson/jenkins/plugins/externalresource/dispatcher/ReleaseRunListener.java b/...java/com/sonyericsson/jenkins/plugins/externalresource/dispatcher/ReleaseRunListener.java
@@ -92,11 +92,10 @@ private void release(AbstractBuild build, ExternalResource buildResource, PrintS
             StashInfo lockInfo = nodeResource.getLocked();
             if (lockInfo != null) {
                 StashResult result = PluginImpl.getInstance().getManager().release(build.getBuiltOn(),
-                        nodeResource, lockInfo.getKey(), Jenkins.getInstance().getRootUrl() + build.getUrl());
+                        nodeResource, lockInfo.getKey(), build.getUrl());
                 if (result != null && result.isOk()) {
                     //Success!
                     logReleaseSuccess(build, buildResource, buildLogger);
-                    nodeResource.setLocked(null);
                 } else {
                     logReleaseFailure(build, buildLogger, nodeResource, result);
                 }

diff --git a/.../java/com/sonyericsson/jenkins/plugins/externalresource/dispatcher/SelectionCriteria.java b/.../java/com/sonyericsson/jenkins/plugins/externalresource/dispatcher/SelectionCriteria.java
@@ -182,7 +182,7 @@ public boolean prebuild(AbstractBuild<?, ?> build,
         }
         //we have a reserved phone, now lock it.
         StashResult lockResult = resourceManager.lock(node, reserved, reservedInfo.getKey(),
-                Jenkins.getInstance().getRootUrl() + build.getUrl());
+                build.getUrl());
         if (lockResult == null || !lockResult.isOk()) {
             AdminNotifier.getInstance().notify(AdminNotifier.MessageType.ERROR, AdminNotifier.OperationType.LOCK,
                     node, reserved, "Could not lock resource, aborting the build: " + buildName);
@@ -192,10 +192,6 @@ public boolean prebuild(AbstractBuild<?, ?> build,
             return false;
         }
         //update the node and build information.
-        StashInfo lockInfo = new StashInfo(lockResult, build.getUrl());
-        // change the reserved info and set lock info.
-        reserved.setLocked(lockInfo);
-        reserved.setReserved(null);
         ExternalResource locked;
         try {
             locked = reserved.clone();

diff --git a/...a/com/sonyericsson/jenkins/plugins/externalresource/dispatcher/data/ExternalResource.java b/...a/com/sonyericsson/jenkins/plugins/externalresource/dispatcher/data/ExternalResource.java
@@ -205,6 +205,9 @@ public StashInfo getLocked() {
      */
     public void setLocked(StashInfo locked) {
         this.locked = locked;
+        if(locked != null){
+            setReserved(null);
+        }
     }
 
     /**

diff --git a/...ns/externalresource/dispatcher/utils/resourcemanagers/DefaultExternalResourceManager.java b/...ns/externalresource/dispatcher/utils/resourcemanagers/DefaultExternalResourceManager.java
@@ -42,18 +42,18 @@ public String getDisplayName() {
     }
 
     @Override
-    public StashResult reserve(Node node, ExternalResource resource, int seconds, String reservedBy) {
+    public StashResult doReserve(Node node, ExternalResource resource, int seconds, String reservedBy) {
 
         return null;
     }
 
     @Override
-    public StashResult lock(Node node, ExternalResource resource, String key, String lockedBy) {
+    public StashResult doLock(Node node, ExternalResource resource, String key, String lockedBy) {
         return null;
     }
 
     @Override
-    public StashResult release(Node node, ExternalResource resource, String key, String releasedBy) {
+    public StashResult doRelease(Node node, ExternalResource resource, String key, String releasedBy) {
         return null;
     }
 

diff --git a/...s/plugins/externalresource/dispatcher/utils/resourcemanagers/ExternalResourceManager.java b/...s/plugins/externalresource/dispatcher/utils/resourcemanagers/ExternalResourceManager.java
@@ -26,14 +26,16 @@
 
 import com.sonyericsson.hudson.plugins.metadata.model.values.AbstractMetadataValue;
 import com.sonyericsson.jenkins.plugins.externalresource.dispatcher.data.ExternalResource;
+import com.sonyericsson.jenkins.plugins.externalresource.dispatcher.data.StashInfo;
 import com.sonyericsson.jenkins.plugins.externalresource.dispatcher.data.StashResult;
 import hudson.ExtensionPoint;
 import hudson.model.Node;
 
 /**
- * Manager for handling reservation of resources by external services. For example the external resources on a slave
- * might be managed by a daemon who handles locking and unlocking of the resources on the slave itself, extend this to
- * provide your own implementation of such a communication interface.
+ * Manager for handling reservation of resources by external services. The Method Template pattern is used to allow for
+ * sub-classes providing their own reservation functionality. For example the external resources on a slave might be 
+ * managed by a daemon who handles locking and unlocking of the resources on the slave itself, and you extend this  
+ * class to provide your own implementation of such a communication interface.
  *
  * @author Robert Sandell &lt;robert.sandell@sonyericsson.com&gt;
  */
@@ -59,7 +61,28 @@ public abstract class ExternalResourceManager implements ExtensionPoint {
      * @param reservedBy a String describing what reserved the resource.
      * @return the result.
      */
-    public abstract StashResult reserve(Node node, ExternalResource resource, int seconds, String reservedBy);
+    public StashResult reserve(Node node, ExternalResource resource, int seconds, String reservedBy){
+      StashResult result = doReserve(node, resource, seconds, reservedBy);
+
+      if (result != null && result.isOk()) {
+        resource.setReserved(new StashInfo(result, reservedBy));
+      }
+
+      return result;
+    }
+
+
+    /**
+     * Implementation of the reservation mechanism itself. Sub-classes can call external sevices to do the actual locking 
+     * if needed.
+     * @param node       the node to communicate with.
+     * @param resource   the resource to reserve.
+     * @param seconds    the number of seconds the lease should be.
+     * @param reservedBy a String describing what reserved the resource.
+     * @return the result.
+     */
+    protected abstract StashResult doReserve(Node node, ExternalResource resource, int seconds, String reservedBy);
+
 
     /**
      * Locks the resource (permanently) until it is unlocked, no other build should be able to use this resource.
@@ -71,7 +94,29 @@ public abstract class ExternalResourceManager implements ExtensionPoint {
      * @param lockedBy a String describing what locked the resource.
      * @return the result.
      */
-    public abstract StashResult lock(Node node, ExternalResource resource, String key, String lockedBy);
+    public StashResult lock(Node node, ExternalResource resource, String key, String lockedBy){
+      StashResult result = doLock(node, resource, key, lockedBy);
+
+      if (result != null && result.isOk()) {
+        resource.setLocked(new StashInfo(result, lockedBy));
+      }
+
+      return result;
+    }
+
+
+    /**
+     * Implementation of the locking mechanism itself. Sub-classes can call external sevices to do the actual locking
+     * if needed.
+     * @param node     the node holding the resource.
+     * @param resource the resource to lock.
+     * @param key      the key to be able to lock it (retained from
+     *                 {@link #reserve(hudson.model.Node, ExternalResource, int, String)}).
+     * @param lockedBy a String describing what locked the resource.
+     * @return the result.
+     */
+    protected abstract StashResult doLock(Node node, ExternalResource resource, String key, String lockedBy);
+
 
     /**
      * Releases the resource, other builds can now use it.
@@ -85,7 +130,31 @@ public abstract class ExternalResourceManager implements ExtensionPoint {
      * @param releasedBy a String describing what released the resource.
      * @return the result.
      */
-    public abstract StashResult release(Node node, ExternalResource resource, String key, String releasedBy);
+    public StashResult release(Node node, ExternalResource resource, String key, String releasedBy){
+      StashResult result = doRelease(node, resource, key, releasedBy);
+
+      if (result != null && result.isOk()) {
+        resource.setReserved(null);
+        resource.setLocked(null);
+      }
+
+      return result;
+    }
+
+
+    /**
+     * Implementation of the releasing mechanism itself. Sub-classes can call external sevices to do the actual locking
+     * if needed.
+     * @param node       the node holding the resource.
+     * @param resource   the resource to unlock.
+     * @param key        the key to unlock the resource with (retained from a previous call to
+     *                   {@link  #lock(hudson.model.Node,
+     *                   com.sonyericsson.jenkins.plugins.externalresource.dispatcher.data.ExternalResource,
+     *                   String, String)}.
+     * @param releasedBy a String describing what released the resource.
+     * @return the result.
+     */
+    protected abstract StashResult doRelease(Node node, ExternalResource resource, String key, String releasedBy);
 
     /**
      * Answers true if these operations are allowed using this ExternalResourceManager.

diff --git a/...ugins/externalresource/dispatcher/utils/resourcemanagers/NoopExternalResourceManager.java b/...ugins/externalresource/dispatcher/utils/resourcemanagers/NoopExternalResourceManager.java
@@ -58,19 +58,19 @@ public String getDisplayName() {
     protected final StashResult okResult = new StashResult("noop", "noop");
 
     @Override
-    public StashResult reserve(Node node, ExternalResource resource, int seconds, String reservedBy) {
+    public StashResult doReserve(Node node, ExternalResource resource, int seconds, String reservedBy) {
         Trigger.timer.schedule(new ReservationTimeoutTask(node.getNodeName(), resource.getId()),
                 TimeUnit.SECONDS.toMillis(seconds));
         return okResult;
     }
 
     @Override
-    public StashResult lock(Node node, ExternalResource resource, String key, String lockedBy) {
+    public StashResult doLock(Node node, ExternalResource resource, String key, String lockedBy) {
         return okResult;
     }
 
     @Override
-    public StashResult release(Node node, ExternalResource resource, String key, String releasedBy) {
+    public StashResult doRelease(Node node, ExternalResource resource, String key, String releasedBy) {
         return okResult;
     }
 

diff --git a/...nalresource/dispatcher/utils/resourcemanagers/ResourceMonitorExternalResourceManager.java b/...nalresource/dispatcher/utils/resourcemanagers/ResourceMonitorExternalResourceManager.java
@@ -151,7 +151,7 @@ public String getDisplayName() {
     }
 
     @Override
-    public StashResult reserve(Node node, ExternalResource resource, int seconds, String reservedBy) {
+    public StashResult doReserve(Node node, ExternalResource resource, int seconds, String reservedBy) {
         RpcResult rpcRes = null;
         String resourceId = getResourceId(resource);
         try {
@@ -200,7 +200,7 @@ public StashResult reserve(Node node, ExternalResource resource, int seconds, St
     }
 
     @Override
-    public StashResult lock(Node node, ExternalResource resource, String key, String lockedBy) {
+    public StashResult doLock(Node node, ExternalResource resource, String key, String lockedBy) {
         RpcResult rpcRes = null;
         String resourceId = getResourceId(resource);
 
@@ -256,7 +256,7 @@ public StashResult lock(Node node, ExternalResource resource, String key, String
     }
 
     @Override
-    public StashResult release(Node node, ExternalResource resource, String key, String releasedBy) {
+    public StashResult doRelease(Node node, ExternalResource resource, String key, String releasedBy) {
         RpcResult rpcRes = null;
         String resourceId = getResourceId(resource);
 

diff --git a/...kins/plugins/externalresource/dispatcher/utils/NoopExternalResourceManagerHudsonTest.java b/...kins/plugins/externalresource/dispatcher/utils/NoopExternalResourceManagerHudsonTest.java
@@ -76,8 +76,8 @@ protected void setUp() throws Exception {
      */
     public void testReserve() {
         StashResult result = PluginImpl.getNoopResourceManager().reserve(slave, resource, 1, "me");
-        resource.setReserved(new StashInfo(result, "me"));
         assertTrue(result.isOk());
+        assertFalse(resource.isAvailable());
         try {
             Thread.sleep(1500);
         } catch (InterruptedException e) {

diff --git a/...ugins/externalresource/dispatcher/utils/resourcemanagers/ExternalResourceManagerTest.java b/...ugins/externalresource/dispatcher/utils/resourcemanagers/ExternalResourceManagerTest.java
@@ -30,6 +30,7 @@
 import static junit.framework.Assert.assertEquals;
 import static junit.framework.Assert.assertNotNull;
 import static junit.framework.Assert.fail;
+import static org.junit.Assert.assertFalse;
 import static org.mockito.Mockito.when;
 
 import com.sonyericsson.jenkins.plugins.externalresource.dispatcher.MockUtils;
@@ -132,6 +133,7 @@ public void testReserve() {
 
         StashResult sRes = rpcCallERM.reserve(n, er, time, "me");
 
+        assertFalse(er.isAvailable());
         assertEquals(code, sRes.getErrorCode());
         assertEquals(message, sRes.getMessage());
         assertEquals(reserveKey, sRes.getKey());
@@ -179,6 +181,7 @@ public void testLock() {
 
         StashResult sRes = rpcCallERM.lock(n, er, reserveKey, "me");
 
+        assertFalse(er.isAvailable());
         assertEquals(code, sRes.getErrorCode());
         assertEquals(message, sRes.getMessage());
         assertEquals(Status.OK, sRes.getStatus());
@@ -243,6 +246,7 @@ public void testRelease() {
 
         StashResult sRes = rpcCallERM.release(n, er, key, "me");
 
+        assertTrue(er.isAvailable());
         assertEquals(code, sRes.getErrorCode());
         assertEquals(Status.OK, sRes.getStatus());
         assertEquals(message, sRes.getMessage());