[FIXED JENKINS-22607] Avoid loading recent builds when deletion of bu…

…ilds older than some date has been requested.
jenkinsci · Apr 14, 2014 · d9188ce · d9188ce
1 parent 3dc0a44
commit d9188ce
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 16 deletions.
diff --git a/changelog.html b/changelog.html
@@ -55,7 +55,9 @@
 <!-- Record your changes in the trunk here. -->
 <div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
 <ul class=image>
-  <li class=>
+  <li class=bug>
+    More efficient deletion of old builds (specified by date).
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-22607">issue 22607</a>)
 </ul>
 </div><!--=TRUNK-END=-->
 

diff --git a/core/src/main/java/hudson/tasks/LogRotator.java b/core/src/main/java/hudson/tasks/LogRotator.java
@@ -118,9 +118,11 @@ public void perform(Job<?,?> job) throws IOException, InterruptedException {
             // Note that RunList.size is deprecated, and indeed here we are loading all the builds of the job.
             // However we would need to load the first numToKeep anyway, just to skip over them;
             // and we would need to load the rest anyway, to delete them.
+            // (Using RunMap.headMap would not suffice, since we do not know if some recent builds have been deleted for other reasons,
+            // so simply subtracting numToKeep from the currently last build number might cause us to delete too many.)
             List<? extends Run<?,?>> builds = job.getBuilds();
             for (Run r : copy(builds.subList(Math.min(builds.size(), numToKeep), builds.size()))) {
-                if (shouldKeepRun(r, lsb, lstb, null)) {
+                if (shouldKeepRun(r, lsb, lstb)) {
                     continue;
                 }
                 LOGGER.log(FINE, "{0} is to be removed", r);
@@ -131,19 +133,23 @@ public void perform(Job<?,?> job) throws IOException, InterruptedException {
         if(daysToKeep!=-1) {
             Calendar cal = new GregorianCalendar();
             cal.add(Calendar.DAY_OF_YEAR,-daysToKeep);
-            for( Run r : copy(job.getBuilds()) ) {
-                if (shouldKeepRun(r, lsb, lstb, cal)) {
-                    continue;
+            Run r = job.getFirstBuild();
+            while (r != null) {
+                if (tooNew(r, cal)) {
+                    break;
                 }
-                LOGGER.log(FINE, "{0} is to be removed", r);
-                r.delete();
+                if (!shouldKeepRun(r, lsb, lstb)) {
+                    LOGGER.log(FINE, "{0} is to be removed", r);
+                    r.delete();
+                }
+                r = r.getNextBuild();
             }
         }
 
         if(artifactNumToKeep!=null && artifactNumToKeep!=-1) {
             List<? extends Run<?,?>> builds = job.getBuilds();
             for (Run r : copy(builds.subList(Math.min(builds.size(), artifactNumToKeep), builds.size()))) {
-                if (shouldKeepRun(r, lsb, lstb, null)) {
+                if (shouldKeepRun(r, lsb, lstb)) {
                     continue;
                 }
                 LOGGER.log(FINE, "{0} is to be purged of artifacts", r);
@@ -154,17 +160,21 @@ public void perform(Job<?,?> job) throws IOException, InterruptedException {
         if(artifactDaysToKeep!=null && artifactDaysToKeep!=-1) {
             Calendar cal = new GregorianCalendar();
             cal.add(Calendar.DAY_OF_YEAR,-artifactDaysToKeep);
-            for( Run r : copy(job.getBuilds())) {
-                if (shouldKeepRun(r, lsb, lstb, cal)) {
-                    continue;
+            Run r = job.getFirstBuild();
+            while (r != null) {
+                if (tooNew(r, cal)) {
+                    break;
                 }
-                LOGGER.log(FINE, "{0} is to be purged of artifacts", r);
-                r.deleteArtifacts();
+                if (!shouldKeepRun(r, lsb, lstb)) {
+                    LOGGER.log(FINE, "{0} is to be purged of artifacts", r);
+                    r.deleteArtifacts();
+                }
+                r = r.getNextBuild();
             }
         }
     }
 
-    private boolean shouldKeepRun(Run r, Run lsb, Run lstb, Calendar cal) {
+    private boolean shouldKeepRun(Run r, Run lsb, Run lstb) {
         if (r.isKeepLog()) {
             LOGGER.log(FINER, "{0} is not to be removed or purged of artifacts because it’s marked as a keeper", r);
             return true;
@@ -177,11 +187,16 @@ private boolean shouldKeepRun(Run r, Run lsb, Run lstb, Calendar cal) {
             LOGGER.log(FINER, "{0} is not to be removed or purged of artifacts because it’s the last stable build", r);
             return true;
         }
-        if (cal != null && !r.getTimestamp().before(cal)) {
+        return false;
+    }
+
+    private boolean tooNew(Run r, Calendar cal) {
+        if (!r.getTimestamp().before(cal)) {
             LOGGER.log(FINER, "{0} is not to be removed or purged of artifacts because it’s still new", r);
             return true;
+        } else {
+            return false;
         }
-        return false;
     }
 
     /**