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.

Sign up for GitHub

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

Jump to bottom

Add cache for commit responses based on hashes #839

Merged
merged 1 commit into from
Mar 28, 2024
Merged

Add cache for commit responses based on hashes #839

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 29 additions & 13 deletions src/main/java/com/cloudbees/jenkins/plugins/bitbucket/client/BitbucketCloudApiClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@
private static final Cache<String, BitbucketTeam> cachedTeam = new Cache<>(6, HOURS);
private static final Cache<String, AvatarImage> cachedAvatar = new Cache<>(6, HOURS);
private static final Cache<String, List<BitbucketCloudRepository>> cachedRepositories = new Cache<>(3, HOURS);
private static final Cache<String, BitbucketCloudCommit> cachedCommits = new Cache<>(24, HOURS);
thomas-boehm-tractive marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a bit inconsistent that the maximum duration of cachedCommits is not configurable via BitbucketCloudEndpoint like teamCacheDuration and repositoriesCacheDuration; but I suppose it's okay because Bitbucket Cloud is expected to return the same information as in the immutable Git commit object. In particular, BitbucketCloudCommit does not include git notes.

private transient BitbucketRepository cachedRepository;
private transient String cachedDefaultBranch;

Expand All @@ -156,12 +157,14 @@
List<String> stats = new ArrayList<>();
stats.add("Team: " + cachedTeam.stats().toString());
stats.add("Repositories : " + cachedRepositories.stats().toString());
stats.add("Commits: " + cachedCommits.stats().toString());
return stats;
}

public static void clearCaches() {
cachedTeam.evictAll();
cachedRepositories.evictAll();
cachedCommits.evictAll();

Check warning on line 167 in src/main/java/com/cloudbees/jenkins/plugins/bitbucket/client/BitbucketCloudApiClient.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 160-167 are not covered by tests
}

@Deprecated
Expand Down Expand Up @@ -519,22 +522,35 @@
@Override
@CheckForNull
public BitbucketCommit resolveCommit(@NonNull String hash) throws IOException, InterruptedException {
String url = UriTemplate.fromTemplate(REPO_URL_TEMPLATE + "/commit/{hash}")
.set("owner", owner)
.set("repo", repositoryName)
.set("hash", hash)
.expand();
String response;
final String url = UriTemplate.fromTemplate(REPO_URL_TEMPLATE + "/commit/{hash}")
.set("owner", owner)
.set("repo", repositoryName)
.set("hash", hash)
.expand();

Callable<BitbucketCloudCommit> request = () -> {
String response;
try {
response = getRequest(url);
} catch (FileNotFoundException e) {
return null;
}
try {
return JsonParser.toJava(response, BitbucketCloudCommit.class);
} catch (IOException e) {
throw new IOException("I/O error when parsing response from URL: " + url, e);
}
};

try {
response = getRequest(url);
} catch (FileNotFoundException e) {
if (enableCache) {

Check warning on line 546 in src/main/java/com/cloudbees/jenkins/plugins/bitbucket/client/BitbucketCloudApiClient.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 546 is only partially covered, one branch is missing
return cachedCommits.get(hash, request);

Check warning on line 547 in src/main/java/com/cloudbees/jenkins/plugins/bitbucket/client/BitbucketCloudApiClient.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 547 is not covered by tests
thomas-boehm-tractive marked this conversation as resolved.
Show resolved Hide resolved
} else {
return request.call();
}
} catch (Exception ex) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I read right this catch eat any exception thrown by the callable. Before exception was log to the build console, now it is not logged and return null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think you might be right. Did you already create a new issue for this, as this PR is already merged?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, I was here because I check changes in jenkins plugin before update in production.
Feel free to create an issue and a PR with a unit test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomas-boehm-tractive would you be able to create a PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomas-boehm-tractive any news about fix for this issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response, I didn't get to this in the last weeks.
Honestly, I am not really sure if this really is an issue, as similar blocks in this file behave exactly like this (see e.g. getTeam or getTeamAvatar function). My PR changed the behavior, but change it to behave like the code does on several other positions.
Are you convinced I should change this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'm sure, what is the reason to eat the exception and not inform the user that something was wrong?
I was on these changes of this pull request, I did see other places. Eventually we should fix also other code.
In short what should do the jenkins user if something happens and there is no apparently reason? I should replicate with jenkins in debug mode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a ticket, and will create a PR within the next days:
#850

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created PR, I think this should bring back the behavior from before this PR was merged: #851

return null;

Check warning on line 552 in src/main/java/com/cloudbees/jenkins/plugins/bitbucket/client/BitbucketCloudApiClient.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 551-552 are not covered by tests
}
try {
return JsonParser.toJava(response, BitbucketCloudCommit.class);
} catch (IOException e) {
throw new IOException("I/O error when parsing response from URL: " + url, e);
}
}

/**
Expand Down