Merge pull request #837 from kutzi/estimated-time

[FIXED JENKINS-18196] Change to the estimated duratrion calculation to prevent performance issues with lazy build loading
jenkinsci · Jul 17, 2013 · 12e9a55 · 12e9a55
2 parents 8438882 + e77441f
commit 12e9a55
Show file tree

Hide file tree

Showing 5 changed files with 111 additions and 19 deletions.
diff --git a/core/src/main/java/hudson/model/Job.java b/core/src/main/java/hudson/model/Job.java
@@ -26,6 +26,7 @@
 import com.google.common.base.Function;
 import com.google.common.collect.Collections2;
 import com.infradna.tool.bridge_method_injector.WithBridgeMethods;
+
 import hudson.EnvVars;
 import hudson.Extension;
 import hudson.ExtensionPoint;
@@ -67,6 +68,7 @@
 import jenkins.util.io.OnMaster;
 import net.sf.json.JSONException;
 import net.sf.json.JSONObject;
+
 import org.jfree.chart.ChartFactory;
 import org.jfree.chart.JFreeChart;
 import org.jfree.chart.axis.CategoryAxis;
@@ -87,6 +89,7 @@
 import org.kohsuke.stapler.interceptor.RequirePOST;
 
 import javax.servlet.ServletException;
+
 import java.awt.*;
 import java.io.*;
 import java.net.URLEncoder;
@@ -898,8 +901,52 @@ public List<RunT> getLastBuildsOverThreshold(int numberOfBuilds, Result threshol
         return result;
     }
 
+    /**
+     * Returns candidate build for calculating the estimated duration of the current run.
+     * 
+     * Returns the 3 last successful (stable or unstable) builds, if there are any.
+     * Failing to find 3 of those, it will return up to 3 last unsuccessful builds.
+     * 
+     * In any case it will not go more than 6 builds into the past to avoid costly build loading.
+     */
+    @SuppressWarnings("unchecked")
+    protected List<RunT> getEstimatedDurationCandidates() {
+        List<RunT> candidates = new ArrayList<RunT>(3);
+        RunT lastSuccessful = (RunT) Permalink.LAST_SUCCESSFUL_BUILD.resolve(this);
+        int lastSuccessfulNumber = -1;
+        if (lastSuccessful != null) {
+            candidates.add(lastSuccessful);
+            lastSuccessfulNumber = lastSuccessful.getNumber();
+        }
+
+        int i = 0;
+        RunT r = (RunT) Permalink.LAST_BUILD.resolve(this);
+        List<RunT> fallbackCandidates = new ArrayList<RunT>(3);
+        while (r != null && candidates.size() < 3 && i < 6) {
+            if (!r.isBuilding() && r.getResult() != null && r.getNumber() != lastSuccessfulNumber) {
+                Result result = r.getResult();
+                if (result.isBetterOrEqualTo(Result.UNSTABLE)) {
+                    candidates.add(r);
+                } else if (result.isCompleteBuild()) {
+                    fallbackCandidates.add(r);
+                }
+            }
+            i++;
+            r = r.getPreviousBuild();
+        }
+
+        while (candidates.size() < 3) {
+            if (fallbackCandidates.isEmpty())
+                break;
+            RunT run = fallbackCandidates.remove(0);
+            candidates.add(run);
+        }
+
+        return candidates;
+    }
+
     public long getEstimatedDuration() {
-        List<RunT> builds = getLastBuildsOverThreshold(3, Result.UNSTABLE);
+        List<RunT> builds = getEstimatedDurationCandidates();
 
         if(builds.isEmpty())     return -1;
 

diff --git a/core/src/main/java/hudson/model/Result.java b/core/src/main/java/hudson/model/Result.java
@@ -48,30 +48,30 @@ public final class Result implements Serializable, CustomExportedBean {
     /**
      * The build had no errors.
      */
-    public static final Result SUCCESS = new Result("SUCCESS",BallColor.BLUE,0);
+    public static final Result SUCCESS = new Result("SUCCESS",BallColor.BLUE,0,true);
     /**
      * The build had some errors but they were not fatal.
      * For example, some tests failed.
      */
-    public static final Result UNSTABLE = new Result("UNSTABLE",BallColor.YELLOW,1);
+    public static final Result UNSTABLE = new Result("UNSTABLE",BallColor.YELLOW,1,true);
     /**
      * The build had a fatal error.
      */
-    public static final Result FAILURE = new Result("FAILURE",BallColor.RED,2);
+    public static final Result FAILURE = new Result("FAILURE",BallColor.RED,2,true);
     /**
      * The module was not built.
      * <p>
      * This status code is used in a multi-stage build (like maven2)
      * where a problem in earlier stage prevented later stages from building.
      */
-    public static final Result NOT_BUILT = new Result("NOT_BUILT",BallColor.NOTBUILT,3);
+    public static final Result NOT_BUILT = new Result("NOT_BUILT",BallColor.NOTBUILT,3,false);
     /**
      * The build was manually aborted.
      *
      * If you are catching {@link InterruptedException} and interpreting it as {@link #ABORTED},
      * you should check {@link Executor#abortResult()} instead (starting 1.417.)
      */
-    public static final Result ABORTED = new Result("ABORTED",BallColor.ABORTED,4);
+    public static final Result ABORTED = new Result("ABORTED",BallColor.ABORTED,4,false);
 
     private final String name;
 
@@ -84,11 +84,17 @@ public final class Result implements Serializable, CustomExportedBean {
      * Default ball color for this status.
      */
     public final BallColor color;
+
+    /**
+     * Is this a complete build - i.e. did it run to the end (not aborted)?
+     */
+    public final boolean completeBuild;
 
-    private Result(String name, BallColor color, int ordinal) {
+    private Result(String name, BallColor color, int ordinal, boolean complete) {
         this.name = name;
         this.color = color;
         this.ordinal = ordinal;
+        this.completeBuild = complete;
     }
 
     /**
@@ -116,6 +122,15 @@ public boolean isBetterThan(Result that) {
     public boolean isBetterOrEqualTo(Result that) {
         return this.ordinal <= that.ordinal;
     }
+
+    /**
+     * Is this a complete build - i.e. did it run to the end (not aborted)?
+     * 
+     * @since TODO
+     */
+    public boolean isCompleteBuild() {
+        return this.completeBuild;
+    }
 
 
     @Override

diff --git a/maven-plugin/src/main/java/hudson/maven/MavenModuleSet.java b/maven-plugin/src/main/java/hudson/maven/MavenModuleSet.java
@@ -1112,6 +1112,11 @@ private List<Queue.Item> filter(Collection<Queue.Item> base) {
     public String getUserConfiguredGoals() {
         return goals;
     }
+
+    @Override
+    protected List<MavenModuleSetBuild> getEstimatedDurationCandidates() {
+        return super.getEstimatedDurationCandidates();
+    }
 
     /*package*/ void reconfigure(PomInfo rootPom) throws IOException {
         if(this.rootModule!=null && this.rootModule.equals(rootPom.name))

diff --git a/maven-plugin/src/main/java/hudson/maven/MavenModuleSetBuild.java b/maven-plugin/src/main/java/hudson/maven/MavenModuleSetBuild.java
@@ -333,7 +333,7 @@ public long getEstimatedDuration() {
      * to the sum of durations of the module builds.
      */
     private long estimateModuleSetBuildDurationOverhead(int numberOfBuilds) {
-        List<MavenModuleSetBuild> moduleSetBuilds = getPreviousBuildsOverThreshold(numberOfBuilds, Result.UNSTABLE);
+        List<MavenModuleSetBuild> moduleSetBuilds = getParent().getEstimatedDurationCandidates();
 
         if (moduleSetBuilds.isEmpty()) {
             return 0;

diff --git a/test/src/test/java/hudson/model/SimpleJobTest.java b/test/src/test/java/hudson/model/SimpleJobTest.java
@@ -4,15 +4,21 @@
 import java.util.SortedMap;
 import java.util.TreeMap;
 
-import junit.framework.Assert;
-import org.jvnet.hudson.test.HudsonTestCase;
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
 
 /**
  * Unit test for {@link Job}.
  */
 @SuppressWarnings("rawtypes")
-public class SimpleJobTest extends HudsonTestCase {
+public class SimpleJobTest {
 
+    @Rule
+    public JenkinsRule rule = new JenkinsRule();
+
+    @Test
     public void testGetEstimatedDuration() throws IOException {
 
         final SortedMap<Integer, TestBuild> runs = new TreeMap<Integer, TestBuild>();
@@ -28,12 +34,13 @@ public void testGetEstimatedDuration() throws IOException {
         TestBuild lastBuild = new TestBuild(project, Result.SUCCESS, 42, previousBuild);
         runs.put(1, lastBuild);
 
-        // without assuming to know to much about the internal calculation
+        // without assuming to know too much about the internal calculation
         // we can only assume that the result is between the maximum and the minimum
-        Assert.assertTrue(project.getEstimatedDuration() < 42);
-        Assert.assertTrue(project.getEstimatedDuration() > 15);
+        Assert.assertTrue("Expected < 42, but was "+project.getEstimatedDuration(), project.getEstimatedDuration() < 42);
+        Assert.assertTrue("Expected > 15, but was "+project.getEstimatedDuration(), project.getEstimatedDuration() > 15);
     }
 
+    @Test
     public void testGetEstimatedDurationWithOneRun() throws IOException {
 
         final SortedMap<Integer, TestBuild> runs = new TreeMap<Integer, TestBuild>();
@@ -58,6 +65,7 @@ public void testGetEstimatedDurationWithFailedRun() throws IOException {
         Assert.assertEquals(-1, project.getEstimatedDuration());
     }
 
+    @Test
     public void testGetEstimatedDurationWithNoRuns() throws IOException {
 
         final SortedMap<Integer, TestBuild> runs = new TreeMap<Integer, TestBuild>();
@@ -67,13 +75,17 @@ public void testGetEstimatedDurationWithNoRuns() throws IOException {
         Assert.assertEquals(-1, project.getEstimatedDuration());
     }
 
+    @Test
     public void testGetEstimatedDurationIfPrevious3BuildsFailed() throws IOException {
 
         final SortedMap<Integer, TestBuild> runs = new TreeMap<Integer, TestBuild>();
 
         Job project = createMockProject(runs);
 
-        TestBuild prev4Build = new TestBuild(project, Result.SUCCESS, 1, null);
+        TestBuild prev5Build = new TestBuild(project, Result.UNSTABLE, 1, null);
+        runs.put(6, prev5Build);
+
+        TestBuild prev4Build = new TestBuild(project, Result.SUCCESS, 1, prev5Build);
         runs.put(5, prev4Build);
 
         TestBuild prev3Build = new TestBuild(project, Result.SUCCESS, 1, prev4Build);
@@ -88,9 +100,21 @@ public void testGetEstimatedDurationIfPrevious3BuildsFailed() throws IOException
         TestBuild lastBuild = new TestBuild(project, Result.FAILURE, 50, previousBuild);
         runs.put(1, lastBuild);
 
-        // failed builds must not be used. Instead the last successful builds before them
-        // must be used
-        Assert.assertEquals(project.getEstimatedDuration(), 1);
+        // failed builds must not be used, if there are succesfulBuilds available.
+        Assert.assertEquals(1, project.getEstimatedDuration());
+    }
+
+    @Test
+    public void testGetEstimatedDurationIfNoSuccessfulBuildTakeDurationOfFailedBuild() throws IOException {
+
+        final SortedMap<Integer, TestBuild> runs = new TreeMap<Integer, TestBuild>();
+
+        Job project = createMockProject(runs);
+
+        TestBuild lastBuild = new TestBuild(project, Result.FAILURE, 50, null);
+        runs.put(1, lastBuild);
+
+        Assert.assertEquals(50, project.getEstimatedDuration());
     }
 
     private Job createMockProject(final SortedMap<Integer, TestBuild> runs) {
@@ -124,13 +148,14 @@ public boolean isBuilding() {
 
     }
 
+    @SuppressWarnings("unchecked")
     private class TestJob extends Job implements TopLevelItem {
 
         int i;
         private final SortedMap<Integer, TestBuild> runs;
 
         public TestJob(SortedMap<Integer, TestBuild> runs) {
-            super(SimpleJobTest.this.jenkins, "name");
+            super(rule.jenkins, "name");
             this.runs = runs;
             i = 1;
         }