Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch implicit exception #150

Merged
merged 3 commits into from May 16, 2014

Conversation

Projects
None yet
4 participants
@rinrinne
Copy link
Member

commented Apr 30, 2014

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

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
@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Apr 30, 2014

plugins » gerrit-trigger-plugin #250 SUCCESS
This pull request looks good

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Apr 30, 2014

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

if (events == null) {
try {
events = CacheBuilder.newBuilder()
.expireAfterWrite(unit.toMillis(expiration), TimeUnit.MILLISECONDS)

This comment has been minimized.

Copy link
@rsandell

rsandell May 5, 2014

Member

Why not use the unit as the second parameter?

This comment has been minimized.

Copy link
@rinrinne

rinrinne May 12, 2014

Author Member

Sure, I just copied this line. I will fix it.

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

This comment has been minimized.

Copy link
@rsandell

rsandell May 5, 2014

Member

unit could be null

This comment has been minimized.

Copy link
@rinrinne

rinrinne May 12, 2014

Author Member

Instead, I will add argument check in constructor then always sets available values.

@rsandell

This comment has been minimized.

Copy link
Member

commented May 5, 2014

There should also be a check for not providing negative integers to the setting on the page.

public static ReplicationCache createCache(long expiration, TimeUnit unit) {
ReplicationCache cache = new ReplicationCache(expiration, unit);
if (!cache.initialize()) {
cache = new ReplicationCache();

This comment has been minimized.

Copy link
@rsandell

rsandell May 5, 2014

Member

even though initialize() performs some logging, it would be nice to have a trace logging here to see the decision path.

This comment has been minimized.

Copy link
@rinrinne

rinrinne May 12, 2014

Author Member

OK, I will do it.

@rinrinne

This comment has been minimized.

Copy link
Member Author

commented May 12, 2014

I will check UI code then try to fix.

rsandell added a commit that referenced this pull request May 16, 2014

@rsandell rsandell merged commit fa9725a into jenkinsci:master May 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.