Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Catch implicit exception
CacheBuilder.expireAfterWrite() has possibility to raise exception if
expiration is negative. So cache object cannot be set to final member in
ReplicationCache. Also its exception is not catched anywhere.

This patch moves cache creation and logging to new method.
And adds factory class for initializing ReplicationCache correctly.

Fix for JENKINS-22814: Some infomations are logged from constructor on
startup 
https://issues.jenkins-ci.org/browse/JENKINS-22814
  • Loading branch information
rinrinne committed Apr 30, 2014
1 parent b8b6d0c commit 12b74ea
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 16 deletions.
Expand Up @@ -40,14 +40,53 @@
*/
public class ReplicationCache {

/**
* A factory class for ReplicationCache.
*/
public static final class Factory {
/**
* Constructor
*/
private Factory() {
}

/**
* Create {@link ReplicationCache}.
*
* @return the instance of {@link ReplicationCache} or null.
*/
public static ReplicationCache createCache() {
ReplicationCache cache = new ReplicationCache();
cache.initialize();
return cache;
}

/**
* Create {@link ReplicationCache}.
*
* @param expiration Cache expiration
* @param unit the unit that expiration is expressed in
* @return the instance of {@link ReplicationCache} or null.
*/
public static ReplicationCache createCache(long expiration, TimeUnit unit) {
ReplicationCache cache = new ReplicationCache(expiration, unit);
if (!cache.initialize()) {
cache = new ReplicationCache();
cache.initialize();
}
return cache;
}
}

/**
* Cache expiration in minutes.
*/
public static final int DEFAULT_EXPIRATION_IN_MINUTES = (int)TimeUnit.HOURS.toMinutes(6);

private static final Logger logger = LoggerFactory.getLogger(ReplicationCache.class);
private final long expirationInMilliseconds;
private final Cache<RefReplicatedId, RefReplicated> events;
private final long expiration;
private final TimeUnit unit;
private Cache<RefReplicatedId, RefReplicated> events = null;

/**
* Default constructor.
Expand All @@ -63,17 +102,37 @@ public ReplicationCache() {
* @param unit the unit that expiration is expressed in
*/
public ReplicationCache(long expiration, TimeUnit unit) {
this.expirationInMilliseconds = unit.toMillis(expiration);
events = CacheBuilder.newBuilder().expireAfterWrite(expirationInMilliseconds, TimeUnit.MILLISECONDS).build();
logger.info("initialized replication cache with expiration in {}: {}", unit, expiration);
this.expiration = expiration;
this.unit = unit;
}

/**
* Initialize cache.
* @return true if success
*/
public boolean initialize() {
if (events == null) {
try {
events = CacheBuilder.newBuilder()
.expireAfterWrite(unit.toMillis(expiration), TimeUnit.MILLISECONDS)
.build();
logger.info("initialized replication cache with expiration in {}: {}", unit, expiration);
} catch (Exception ex) {
logger.warn("initialize failure in {}: {}", unit, expiration);
return false;
}
}
return true;
}

/**
* Cache the specified RefReplicated.
* @param refReplicated the event to cache
*/
public void put(RefReplicated refReplicated) {
events.put(RefReplicatedId.fromRefReplicated(refReplicated), refReplicated);
if (events != null) {
events.put(RefReplicatedId.fromRefReplicated(refReplicated), refReplicated);
}
}

/**
Expand All @@ -82,7 +141,7 @@ public void put(RefReplicated refReplicated) {
* @return true if expired, otherwise false
*/
public boolean isExpired(long timestamp) {
return (System.currentTimeMillis() - timestamp) > expirationInMilliseconds;
return (System.currentTimeMillis() - timestamp) > unit.toMillis(expiration);
}

/**
Expand All @@ -94,8 +153,12 @@ public boolean isExpired(long timestamp) {
* @return the RefReplicated if found, otherwise null
*/
public RefReplicated getIfPresent(String gerritServer, String gerritProject, String ref, String slaveHost) {
RefReplicatedId refReplicatedId = new RefReplicatedId(gerritServer, gerritProject, ref, slaveHost);
return events.getIfPresent(refReplicatedId);
if (events != null) {
RefReplicatedId refReplicatedId = new RefReplicatedId(gerritServer, gerritProject, ref, slaveHost);
return events.getIfPresent(refReplicatedId);
} else {
return null;
}
}

/**
Expand Down
Expand Up @@ -69,8 +69,9 @@ public class ReplicationQueueTaskDispatcher extends QueueTaskDispatcher implemen
*/
public ReplicationQueueTaskDispatcher() {
this(PluginImpl.getInstance().getHandler(),
new ReplicationCache(PluginImpl.getInstance().getPluginConfig().getReplicationCacheExpirationInMinutes(),
TimeUnit.MINUTES));
ReplicationCache.Factory.createCache(
PluginImpl.getInstance().getPluginConfig().getReplicationCacheExpirationInMinutes(),
TimeUnit.MINUTES));
}

/**
Expand Down
Expand Up @@ -48,7 +48,7 @@ public class ReplicationCacheTest {
*/
@Test
public void shouldReturnCachedEvent() {
ReplicationCache replicationCache = new ReplicationCache();
ReplicationCache replicationCache = ReplicationCache.Factory.createCache();
RefReplicated refReplicated = Setup.createRefReplicatedEvent("someProject", "refs/changes/1/1/1", "someServer",
"someSlave", null);
replicationCache.put(refReplicated);
Expand All @@ -62,7 +62,7 @@ public void shouldReturnCachedEvent() {
*/
@Test
public void shouldReturnNullWhenNoCachedEventFound() {
ReplicationCache replicationCache = new ReplicationCache();
ReplicationCache replicationCache = ReplicationCache.Factory.createCache();
assertNull(replicationCache.getIfPresent("someServer", "someProject", "refs/changes/1/1/1", "someSlave"));
}

Expand All @@ -72,7 +72,7 @@ public void shouldReturnNullWhenNoCachedEventFound() {
*/
@Test
public void shouldEvictExpiredEvent() throws InterruptedException {
ReplicationCache replicationCache = new ReplicationCache(100, TimeUnit.MILLISECONDS);
ReplicationCache replicationCache = ReplicationCache.Factory.createCache(100, TimeUnit.MILLISECONDS);
RefReplicated refReplicated = Setup.createRefReplicatedEvent("someProject", "refs/changes/1/1/1", "someServer",
"someSlave", null);
replicationCache.put(refReplicated);
Expand All @@ -92,7 +92,7 @@ public void shouldEvictExpiredEvent() throws InterruptedException {
*/
@Test
public void shouldReturnIsExpired() {
ReplicationCache replicationCache = new ReplicationCache(100, TimeUnit.MILLISECONDS);
ReplicationCache replicationCache = ReplicationCache.Factory.createCache(100, TimeUnit.MILLISECONDS);
assertFalse(replicationCache.isExpired(System.currentTimeMillis()));
assertTrue(replicationCache.isExpired(System.currentTimeMillis() - 200));
}
Expand Down
Expand Up @@ -89,7 +89,7 @@ public class ReplicationQueueTaskDispatcherTest {
@Before
public void setUp() {
gerritHandlerMock = mock(GerritHandler.class);
dispatcher = new ReplicationQueueTaskDispatcher(gerritHandlerMock, new ReplicationCache());
dispatcher = new ReplicationQueueTaskDispatcher(gerritHandlerMock, ReplicationCache.Factory.createCache());
gerritTriggerMock = mock(GerritTrigger.class);
queueMock = mock(Queue.class);
Jenkins jenkinsMock = mock(Jenkins.class);
Expand Down

0 comments on commit 12b74ea

Please sign in to comment.