Skip to content
Permalink
Browse files
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 12b74ea3eb73145c242c75dcf4a35a6472a329c5
@@ -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.
@@ -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);
}
}

/**
@@ -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);
}

/**
@@ -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;
}
}

/**
@@ -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));
}

/**
@@ -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);
@@ -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"));
}

@@ -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);
@@ -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));
}
@@ -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);

0 comments on commit 12b74ea

Please sign in to comment.