HTTPCLIENT-2277: optimization of internal cache operations#464
Conversation
arturobernalg
left a comment
arturobernalg
left a comment
There was a problem hiding this comment.
@ok2c
The change-set generally looks solid. I've made a few observations and have some doubts.
| public String getCacheKey() { | ||
| return cacheKey; | ||
| } | ||
| final CacheHit hit; |
There was a problem hiding this comment.
Shouldn't this class be fully immutable?
There was a problem hiding this comment.
@arturobernalg It is. All its instance variables are final and immutable. There are no access methods but that does not make the class mutable.
| final CacheHit root = result != null ? result.root : null; | ||
|
|
||
| final SimpleHttpResponse fatalErrorResponse = getFatallyNonCompliantResponse(request, context, entry != null); | ||
| final SimpleHttpResponse fatalErrorResponse = getFatallyNonCompliantResponse(request, context, hit != null); |
There was a problem hiding this comment.
I've noticed that the hit != null condition is being checked numerous times in the code. To enhance code readability and reduce redundancy, I suggest encapsulating this logic within the CacheMatch class itself. We could add a hasHit() method as follows:
public boolean hasHit() {
return hit != null;
}
This way, we can simply call cacheMatch.hasHit() wherever we need to check if hit is not null, improving the code's readability and maintainability."
There was a problem hiding this comment.
@arturobernalg This is a matter of taste. I find hit != null clearer but it is just me.
| if (staleIfErrorAppliesTo(statusCode) | ||
| && !staleResponseNotAllowed(requestCacheControl, responseCacheControl, cacheEntry, getCurrentDate()) | ||
| && validityPolicy.mayReturnStaleIfError(requestCacheControl, responseCacheControl, cacheEntry, responseDate)) { | ||
| && !staleResponseNotAllowed(requestCacheControl, responseCacheControl, hit, getCurrentDate()) |
There was a problem hiding this comment.
I noticed in the staleResponseNotAllowed method we are passing in the complete CacheHit object, but only the entry attribute within it is being used. To maintain the original structure would it be feasible to pass just the entry object instead?
| final String variantKey; | ||
| final HttpCacheEntry entry; | ||
|
|
||
| public CacheHit(final String rootKey, final String variantKey, final HttpCacheEntry entry) { |
There was a problem hiding this comment.
Shouldn't this class be fully immutable? Also, shouldn't rootKey and entry be null-safe?
There was a problem hiding this comment.
@arturobernalg The class is immutable. It is also internal so I forwent getters and null checks for its instance variables.
| final SimpleHttpResponse response = responseGenerator.generateResponse(request, responseEntry); | ||
| responseCache.reuseVariantEntryFor(target, request, backendResponse, responseEntry, requestDate, responseDate); | ||
| final SimpleHttpResponse response = responseGenerator.generateResponse(request, hit.entry); | ||
| responseCache.storeReusing(hit, target, request, backendResponse, requestDate, responseDate); |
There was a problem hiding this comment.
Shouldn't be the entry instead of the hit?
| } else { | ||
| LOG.debug("Backend response is not cacheable"); | ||
| responseCache.flushCacheEntriesFor(target, request, new FutureCallback<Boolean>() { | ||
| if (!Method.isSafe(request.getMethod())) { |
There was a problem hiding this comment.
Perhaps we could consider delegating this check (!Method.isSafe(request.getMethod())) inside the method of the class HttpAsyncCache and BasicHttpCache itself. This way, the method becomes more resilient and we ensure the check is always performed when needed, reducing the potential for errors.
There was a problem hiding this comment.
@arturobernalg I am not sure. I do not think this is a cache storage layer decision whether a method is considered safe or not. But let's discuss it when I finish refactoring the cache eviction logic.
|
@arturobernalg Please do another pass. |
arturobernalg
left a comment
arturobernalg
left a comment
There was a problem hiding this comment.
@ok2c the change-set looks good.
2 participants
This is a complete redesign and re-write of the internal cache implementations intended
@arturobernalg Please review.