JENKINS-44579 avoid performance issues on delete by lazily removing f…

…avorites that have been deleted
jenkinsci · Jun 1, 2017 · 3c1e5ac · 3c1e5ac
1 parent 2a30820
commit 3c1e5ac
Show file tree

Hide file tree

Showing 5 changed files with 128 additions and 20 deletions.
diff --git a/pom.xml b/pom.xml
@@ -70,6 +70,11 @@
             <artifactId>token-macro</artifactId>
             <version>2.0</version>
         </dependency>
+        <dependency>
+            <groupId>org.jenkins-ci.plugins</groupId>
+            <artifactId>cloudbees-folder</artifactId>
+            <version>6.0.4</version>
+        </dependency>
     </dependencies>
 
 </project>  

diff --git a/src/main/java/hudson/plugins/favorite/user/Clock.java b/src/main/java/hudson/plugins/favorite/user/Clock.java
@@ -0,0 +1,20 @@
+package hudson.plugins.favorite.user;
+
+import com.google.common.annotations.VisibleForTesting;
+
+class Clock {
+    private final Long time;
+
+    public Clock() {
+        this(Long.MIN_VALUE);
+    }
+
+    @VisibleForTesting
+    public Clock(Long time) {
+        this.time = time;
+    }
+
+    public long getTime() {
+        return time == Long.MIN_VALUE ? System.currentTimeMillis() : time;
+    }
+}
diff --git a/src/main/java/hudson/plugins/favorite/user/FavoriteJobListener.java b/src/main/java/hudson/plugins/favorite/user/FavoriteJobListener.java
@@ -5,7 +5,6 @@
 import hudson.model.Item;
 import hudson.model.User;
 import hudson.model.listeners.ItemListener;
-import hudson.plugins.favorite.FavoritePlugin;
 
 import java.io.IOException;
 import java.util.logging.Level;
@@ -32,20 +31,4 @@ public void onRenamed(Item item, String oldName, String newName) {
       }
     }
   }
-
-  @Override
-  public void onDeleted(Item item) {
-    if (item instanceof AbstractProject<?, ?>) {
-      for (User user : User.getAll()) {
-        FavoriteUserProperty fup = user.getProperty(FavoriteUserProperty.class);
-        try {
-          if (fup != null) {
-            fup.deleteFavourite(item.getFullName());
-          }
-        } catch (IOException e) {
-          LOGGER.log(Level.WARNING, "Remove favourites deleted item <" + item.getFullName() + ">.", e);
-        }
-      }
-    }
-  }
 }
diff --git a/src/main/java/hudson/plugins/favorite/user/FavoriteUserProperty.java b/src/main/java/hudson/plugins/favorite/user/FavoriteUserProperty.java
@@ -1,14 +1,16 @@
 package hudson.plugins.favorite.user;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Maps;
 import hudson.Extension;
 import hudson.model.Descriptor;
 import hudson.model.UserProperty;
 import hudson.model.UserPropertyDescriptor;
 import hudson.plugins.favorite.assets.Asset;
+import jenkins.model.Jenkins;
 import net.sf.json.JSONObject;
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.StaplerRequest;
@@ -18,10 +20,12 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
 import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.TimeUnit;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 /**
  * Do not use directly.
@@ -30,6 +34,8 @@
 @ExportedBean(defaultVisibility = 999)
 public class FavoriteUserProperty extends UserProperty {
 
+    private static final Logger LOGGER = Logger.getLogger(FavoriteUserProperty.class.getName());
+
     @Extension
     public static final UserPropertyDescriptor DESCRIPTOR = new FavoriteUserPropertyDescriptor();
 
@@ -41,12 +47,17 @@ public FavoriteUserProperty() {}
 
     private ConcurrentMap<String, Boolean> data = Maps.newConcurrentMap();
 
+    private transient long lastValidated = 0;
+
+    private transient Clock clock = new Clock();
+
     /**
      * Use {#getAllFavorites()}
      * @return favorites
      */
     @Deprecated
     public List<String> getFavorites() {
+        validateFavoritesExist();
         return ImmutableList.copyOf(Maps.filterEntries(data, new Predicate<Entry<String, Boolean>>() {
             @Override
             public boolean apply(@Nullable Entry<String, Boolean> input) {
@@ -56,6 +67,7 @@ public boolean apply(@Nullable Entry<String, Boolean> input) {
     }
 
     public Set<String> getAllFavorites() {
+        validateFavoritesExist();
         return Maps.filterEntries(data, new Predicate<Entry<String, Boolean>>() {
             @Override
             public boolean apply(@Nullable Entry<String, Boolean> input) {
@@ -130,6 +142,27 @@ Object readResolve() {
         return this;
     }
 
+    private void validateFavoritesExist() {
+        if (lastValidated + TimeUnit.HOURS.toMillis(1) > clock.getTime()) {
+            lastValidated = System.currentTimeMillis();
+            Jenkins jenkins = Jenkins.getInstance();
+            for (String fullName : ImmutableSet.copyOf(data.keySet())) {
+                if (jenkins.getItem(fullName) == null) {
+                    try {
+                        deleteFavourite(fullName);
+                    } catch (IOException e) {
+                        LOGGER.log(Level.SEVERE, "Could not purge favorite '" + fullName + "'", e);
+                    }
+                }
+            }
+        }
+    }
+
+    @VisibleForTesting
+    public void setClock(Clock clock) {
+        this.clock = clock;
+    }
+
     public Class getAssetClass() {
         return Asset.class;
     }

diff --git a/src/test/java/hudson/plugins/favorite/user/FavoriteUserPropertyTest.java b/src/test/java/hudson/plugins/favorite/user/FavoriteUserPropertyTest.java
@@ -1,16 +1,39 @@
 package hudson.plugins.favorite.user;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import hudson.model.User;
+import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+
+import java.io.IOException;
+import java.util.concurrent.TimeUnit;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 public class FavoriteUserPropertyTest {
+
+    @Rule
+    public JenkinsRule rule = new JenkinsRule();
+
+    User bob;
+
+    FavoriteUserProperty property;
+
+    @Before
+    public void createUserAndProperty() throws IOException {
+        bob = User.get("bob");
+        property = new FavoriteUserProperty();
+        bob.addProperty(property);
+    }
+
     @Test
     public void testMigration() throws Exception {
-        FavoriteUserProperty property = new FavoriteUserProperty();
         property.favorites.add("foo");
         property.favorites.add("bar");
         property.favorites.add("baz");
@@ -27,4 +50,48 @@ public void testMigration() throws Exception {
 
         assertEquals(ImmutableList.copyOf(property.getAllFavorites()), ImmutableList.copyOf(property.getFavorites()));
     }
+
+    @Test
+    public void lazyDeletionGetAllFavorites() throws Exception {
+        rule.createFreeStyleProject("d");
+
+        property.addFavorite("a");
+        property.addFavorite("b");
+        property.addFavorite("c");
+        property.addFavorite("d");
+
+        // Current time + 1 year
+        property.setClock(new Clock(System.currentTimeMillis() + TimeUnit.DAYS.toMillis(265)));
+
+        // Data should be retained
+        assertEquals(Sets.newHashSet("a", "b", "c", "d"), property.getAllFavorites());
+
+        // Set to one second after epoc
+        property.setClock(new Clock(1L));
+
+        // a,b,c do not exist and should be removed
+        assertEquals(Sets.newHashSet("d"), property.getAllFavorites());
+    }
+
+    @Test
+    public void lazyDeletionGetFavorites() throws Exception {
+        rule.createFreeStyleProject("d");
+
+        property.addFavorite("a");
+        property.addFavorite("b");
+        property.addFavorite("c");
+        property.addFavorite("d");
+
+        // Current time + 1 year
+        property.setClock(new Clock(System.currentTimeMillis() + TimeUnit.DAYS.toMillis(265)));
+
+        // Data should be retained
+        assertEquals(ImmutableSet.of("a", "b", "c", "d"), ImmutableSet.copyOf(property.getFavorites()));
+
+        // Set to one second after epoc
+        property.setClock(new Clock(1L));
+
+        // a,b,c do not exist and should be removed
+        assertEquals(ImmutableSet.of("d"), ImmutableSet.copyOf(property.getFavorites()));
+    }
 }