From 13570a4d8cd9fc730ed8e343c6a72c7d2f5b16af Mon Sep 17 00:00:00 2001 From: Joe Hansche Date: Wed, 10 Jul 2019 20:17:49 -0400 Subject: [PATCH] Improve caching by leveraging atlassian-cache This will enable caching in a clustered environment, such as Jira Data Center. --- .../jira/gerrit/data/IssueReviewsCache.java | 105 ------------------ .../gerrit/data/IssueReviewsCacheLoader.java | 84 ++++++++++++++ .../jira/gerrit/data/IssueReviewsImpl.java | 101 +++++++---------- src/main/resources/atlassian-plugin.xml | 4 + .../gerrit/data/IssueReviewsManagerTest.java | 51 +++++---- 5 files changed, 158 insertions(+), 187 deletions(-) delete mode 100644 src/main/java/com/meetme/plugins/jira/gerrit/data/IssueReviewsCache.java create mode 100644 src/main/java/com/meetme/plugins/jira/gerrit/data/IssueReviewsCacheLoader.java diff --git a/src/main/java/com/meetme/plugins/jira/gerrit/data/IssueReviewsCache.java b/src/main/java/com/meetme/plugins/jira/gerrit/data/IssueReviewsCache.java deleted file mode 100644 index af3c53a..0000000 --- a/src/main/java/com/meetme/plugins/jira/gerrit/data/IssueReviewsCache.java +++ /dev/null @@ -1,105 +0,0 @@ -package com.meetme.plugins.jira.gerrit.data; - -import com.meetme.plugins.jira.gerrit.data.dto.GerritChange; - -import java.util.Collections; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; - -/** - * Created by jhansche on 9/2/16. - */ -public class IssueReviewsCache { - /** Max number of items to retain in the cache */ - private static final int CACHE_CAPACITY = 30; - - /** Number of milliseconds an item may stay in cache: 30 seconds */ - private static final long CACHE_EXPIRATION = 30000; - - /** - * LRU (least recently used) Cache object to avoid slamming the Gerrit server too many times. - *

- * XXX: This might result in an issue using a stale cache for reviews that change often, but - * corresponding issues viewed rarely! To account for that, we also have a cache expiration, so - * that at least after the cache expires, it'll get back in sync. - */ - protected static final Map> lruCache = Collections.synchronizedMap(new TimedCache(CACHE_CAPACITY, CACHE_EXPIRATION)); - - private static IssueReviewsCache sInstance; - - public static synchronized Map> getCache() { - return lruCache; - } - - private static class TimedCache extends LinkedHashMap> { - private static final long serialVersionUID = 296909003142207307L; - - private final int capacity; - private final Map timestamps; - private final long expiration; - - public TimedCache(final int capacity, final long expiration) { - super(capacity + 1, 1.0f, true); - this.capacity = capacity; - this.timestamps = new LinkedHashMap<>(capacity + 1, 1.0f, true); - this.expiration = expiration; - } - - /** - * Returns true if the requested key has expired from the cache. - * - * @param key the key - * @return whether the associated value exists and has expired - */ - private boolean hasKeyExpired(Object key) { - if (timestamps.containsKey(key)) { - Long lastUpdatedTimestamp = timestamps.get(key); - return lastUpdatedTimestamp <= System.currentTimeMillis() - expiration; - } - - return false; - } - - @Override - public boolean containsKey(Object key) { - if (hasKeyExpired(key)) { - this.remove(key); - return false; - } - - return super.containsKey(key); - } - - @Override - public List get(Object key) { - if (hasKeyExpired(key)) { - this.remove(key); - return null; - } - - return super.get(key); - } - - /** - * Removes the eldest entry from the cache if 1) it exceeds the max capacity, or 2) it has - * been in cache for too long. - */ - @Override - protected boolean removeEldestEntry(final Map.Entry> eldest) { - return super.size() > capacity || timestamps.get(eldest.getKey()) <= System.currentTimeMillis() - expiration; - } - - @Override - public List put(String key, List value) { - timestamps.put(key, System.currentTimeMillis()); - return super.put(key, value); - } - - @Override - public List remove(Object key) { - timestamps.remove(key); - return super.remove(key); - } - } -} diff --git a/src/main/java/com/meetme/plugins/jira/gerrit/data/IssueReviewsCacheLoader.java b/src/main/java/com/meetme/plugins/jira/gerrit/data/IssueReviewsCacheLoader.java new file mode 100644 index 0000000..b5ba641 --- /dev/null +++ b/src/main/java/com/meetme/plugins/jira/gerrit/data/IssueReviewsCacheLoader.java @@ -0,0 +1,84 @@ +package com.meetme.plugins.jira.gerrit.data; + +import com.meetme.plugins.jira.gerrit.data.dto.GerritChange; + +import com.atlassian.cache.CacheException; +import com.atlassian.cache.CacheLoader; +import com.sonyericsson.hudson.plugins.gerrit.gerritevents.GerritQueryException; +import com.sonyericsson.hudson.plugins.gerrit.gerritevents.GerritQueryHandler; +import com.sonyericsson.hudson.plugins.gerrit.gerritevents.ssh.Authentication; +import com.sonyericsson.hudson.plugins.gerrit.gerritevents.ssh.SshException; + +import net.sf.json.JSONObject; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Locale; + +import javax.annotation.Nonnull; + +public class IssueReviewsCacheLoader implements CacheLoader> { + private final Logger log = LoggerFactory.getLogger(IssueReviewsCacheLoader.class); + private final GerritConfiguration configuration; + + public IssueReviewsCacheLoader(GerritConfiguration configuration) { + this.configuration = configuration; + } + + @Nonnull + @Override + public List load(@Nonnull String key) { + String query = String.format(Locale.US, configuration.getIssueSearchQuery(), key); + + try { + return getReviewsFromGerrit(query); + } catch (GerritQueryException e) { + log.error("Error querying for issues", e); + throw new CacheException("Error querying for issues: " + e.getMessage(), e); + } + } + + protected List getReviewsFromGerrit(String searchQuery) throws GerritQueryException { + List changes; + + if (!configuration.isSshValid()) { + // return Collections.emptyList(); + throw new GerritConfiguration.NotConfiguredException("Not configured for SSH access"); + } + + Authentication auth = new Authentication(configuration.getSshPrivateKey(), configuration.getSshUsername()); + GerritQueryHandler query = new GerritQueryHandler(configuration.getSshHostname(), configuration.getSshPort(), null, auth); + List reviews; + + try { + reviews = query.queryJava(searchQuery, false, true, false); + } catch (SshException e) { + throw new GerritQueryException("An ssh error occurred while querying for reviews.", e); + } catch (IOException e) { + throw new GerritQueryException("An error occurred while querying for reviews.", e); + } + + changes = new ArrayList<>(reviews.size()); + + for (JSONObject obj : reviews) { + if (obj.has("type") && "stats".equalsIgnoreCase(obj.getString("type"))) { + // The final JSON object in the query results is just a set of statistics + if (log.isDebugEnabled()) { + log.trace("Results from QUERY: " + obj.optString("rowCount", "(unknown)") + " rows; runtime: " + + obj.optString("runTimeMilliseconds", "(unknown)") + " ms"); + } + continue; + } + + changes.add(new GerritChange(obj)); + } + + Collections.sort(changes); + return changes; + } +} diff --git a/src/main/java/com/meetme/plugins/jira/gerrit/data/IssueReviewsImpl.java b/src/main/java/com/meetme/plugins/jira/gerrit/data/IssueReviewsImpl.java index 6387e8b..f57c2c0 100644 --- a/src/main/java/com/meetme/plugins/jira/gerrit/data/IssueReviewsImpl.java +++ b/src/main/java/com/meetme/plugins/jira/gerrit/data/IssueReviewsImpl.java @@ -15,34 +15,52 @@ import com.meetme.plugins.jira.gerrit.data.dto.GerritChange; +import com.atlassian.cache.Cache; +import com.atlassian.cache.CacheException; +import com.atlassian.cache.CacheManager; +import com.atlassian.cache.CacheSettingsBuilder; import com.atlassian.core.user.preferences.Preferences; import com.atlassian.jira.issue.Issue; import com.atlassian.jira.issue.IssueManager; import com.sonyericsson.hudson.plugins.gerrit.gerritevents.GerritQueryException; -import com.sonyericsson.hudson.plugins.gerrit.gerritevents.GerritQueryHandler; -import com.sonyericsson.hudson.plugins.gerrit.gerritevents.ssh.Authentication; -import com.sonyericsson.hudson.plugins.gerrit.gerritevents.ssh.SshException; - -import net.sf.json.JSONObject; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; -import java.util.*; +import java.util.ArrayList; +import java.util.List; +import java.util.Set; +import java.util.concurrent.TimeUnit; public class IssueReviewsImpl implements IssueReviewsManager { private static final Logger log = LoggerFactory.getLogger(IssueReviewsImpl.class); - private final Map> lruCache; + + private final Cache> cache; private GerritConfiguration configuration; private IssueManager jiraIssueManager; - public IssueReviewsImpl(GerritConfiguration configuration, IssueManager jiraIssueManager) { + public IssueReviewsImpl( + GerritConfiguration configuration, + IssueManager jiraIssueManager, + CacheManager cacheManager, + IssueReviewsCacheLoader cacheLoader + ) { this.configuration = configuration; this.jiraIssueManager = jiraIssueManager; - this.lruCache = IssueReviewsCache.getCache(); + this.cache = cacheManager.getCache( + IssueReviewsManager.class.getName() + ".issueChanges.cache", + cacheLoader, //new IssueReviewsCacheLoader(configuration), + new CacheSettingsBuilder() + .flushable() + .statisticsEnabled() + .maxEntries(100) + .replicateAsynchronously() + .expireAfterAccess(30, TimeUnit.MINUTES) + .build() + ); } @Override @@ -56,60 +74,23 @@ public List getReviewsForIssue(Issue issue) throws GerritQueryExce Set allIssueKeys = getIssueKeys(issue); for (String key : allIssueKeys) { - List changes; - - if (lruCache.containsKey(key)) { - log.debug("Getting issues from cache"); - changes = lruCache.get(key); - } else { - log.debug("Getting issues from Gerrit"); - changes = getReviewsFromGerrit(String.format(configuration.getIssueSearchQuery(), key)); - lruCache.put(key, changes); - } - - gerritChanges.addAll(changes); - } - - return gerritChanges; - } - - protected List getReviewsFromGerrit(String searchQuery) throws GerritQueryException { - List changes; - - if (!configuration.isSshValid()) { - // return Collections.emptyList(); - throw new GerritConfiguration.NotConfiguredException("Not configured for SSH access"); - } - - Authentication auth = new Authentication(configuration.getSshPrivateKey(), configuration.getSshUsername()); - GerritQueryHandler query = new GerritQueryHandler(configuration.getSshHostname(), configuration.getSshPort(), null, auth); - List reviews; - - try { - reviews = query.queryJava(searchQuery, false, true, false); - } catch (SshException e) { - throw new GerritQueryException("An ssh error occurred while querying for reviews.", e); - } catch (IOException e) { - throw new GerritQueryException("An error occurred while querying for reviews.", e); - } - - changes = new ArrayList<>(reviews.size()); - - for (JSONObject obj : reviews) { - if (obj.has("type") && "stats".equalsIgnoreCase(obj.getString("type"))) { - // The final JSON object in the query results is just a set of statistics - if (log.isDebugEnabled()) { - log.trace("Results from QUERY: " + obj.optString("rowCount", "(unknown)") + " rows; runtime: " - + obj.optString("runTimeMilliseconds", "(unknown)") + " ms"); + try { + List changes = cache.get(key); + if (changes != null) gerritChanges.addAll(changes); + } catch (CacheException exc) { + if (exc.getCause() instanceof GerritQueryException) { + // TODO: is this really necessary? + // If we swallow the error, then there's no indication on the UI that an error occurred. + // The CacheLoader has to wrap the underlying exception in CacheException in order to throw it. + throw (GerritQueryException) exc.getCause(); } - continue; - } - changes.add(new GerritChange(obj)); + log.error("Error fetching from cache", exc); + throw exc; + } } - Collections.sort(changes); - return changes; + return gerritChanges; } @Override @@ -128,7 +109,7 @@ public boolean doApprovals(Issue issue, List changes, String args, } // Something probably changed! - lruCache.remove(issueKey); + cache.remove(issueKey); } return result; diff --git a/src/main/resources/atlassian-plugin.xml b/src/main/resources/atlassian-plugin.xml index 1cf58f5..5248c14 100644 --- a/src/main/resources/atlassian-plugin.xml +++ b/src/main/resources/atlassian-plugin.xml @@ -48,6 +48,10 @@ com.meetme.plugins.jira.gerrit.data.IssueReviewsManager + + Populates the cached list of reviews per issue. + + > mockCache; + private IssueReviewsManager issueReviewsManager; @Before @@ -71,26 +81,23 @@ public void setUp() { when(mockJiraIssueManager.getAllIssueKeys(mockIssue.getId())).thenReturn(allIssueKeys); // mock gerrit review retrieval - issueReviewsManager = new IssueReviewsImpl(configuration, mockJiraIssueManager) { - @Override - protected List getReviewsFromGerrit(String searchQuery) throws GerritQueryException { - List reviews = new ArrayList<>(); - - if (searchQuery.contains(ISSUE_KEY_OLD)) { - GerritChange oldChangeMock = mock(GerritChange.class); - when(oldChangeMock.getSubject()).thenReturn(ISSUE_KEY_OLD); - reviews.add(oldChangeMock); - } - - if (searchQuery.contains(ISSUE_KEY_NEW)) { - GerritChange newChangeMock = mock(GerritChange.class); - when(newChangeMock.getSubject()).thenReturn(ISSUE_KEY_NEW); - reviews.add(newChangeMock); - } - - return reviews; - } - }; + when(mockCacheManager.getCache( + eq("com.meetme.plugins.jira.gerrit.data.IssueReviewsManager.issueChanges.cache"), + Mockito.>>any(), + any() + )).thenReturn(mockCache); + issueReviewsManager = new IssueReviewsImpl(configuration, mockJiraIssueManager, mockCacheManager, null); + + GerritChange oldChange = createMockChange(ISSUE_KEY_OLD); + GerritChange newChange = createMockChange(ISSUE_KEY_NEW); + when(mockCache.get(eq(ISSUE_KEY_OLD))).thenReturn(Collections.singletonList(oldChange)); + when(mockCache.get(eq(ISSUE_KEY_NEW))).thenReturn(Collections.singletonList(newChange)); + } + + private GerritChange createMockChange(String key) { + GerritChange change = mock(GerritChange.class); + when(change.getSubject()).thenReturn(key); + return change; } @Test