arturobernalg commented on code in PR #466:
URL:
https://github.com/apache/httpcomponents-client/pull/466#discussion_r1267110875
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java:
##########
@@ -456,57 +457,122 @@ public Cancellable storeReusing(
return store(request, originResponse, requestSent, responseReceived,
rootKey, hit.entry, callback);
}
- @Override
- public Cancellable flushCacheEntriesFor(
- final HttpHost host, final HttpRequest request, final
FutureCallback<Boolean> callback) {
- final String rootKey = cacheKeyGenerator.generateKey(host, request);
- if (LOG.isDebugEnabled()) {
- LOG.debug("Flush cache entries: {}", rootKey);
- }
- return storage.removeEntry(rootKey, new FutureCallback<Boolean>() {
+ private void evictEntry(final String cacheKey) {
+ storage.removeEntry(cacheKey, new FutureCallback<Boolean>() {
@Override
public void completed(final Boolean result) {
- callback.completed(result);
}
@Override
public void failed(final Exception ex) {
- if (ex instanceof ResourceIOException) {
- if (LOG.isWarnEnabled()) {
- LOG.warn("I/O error removing cache entry with key {}",
rootKey);
+ if (LOG.isWarnEnabled()) {
+ if (ex instanceof ResourceIOException) {
+ LOG.warn("I/O error removing cache entry with key {}",
cacheKey);
+ } else {
+ LOG.warn("Unexpected error removing cache entry with
key {}", cacheKey, ex);
}
- callback.completed(Boolean.TRUE);
- } else {
- callback.failed(ex);
}
}
@Override
public void cancelled() {
- callback.cancelled();
}
});
}
- @Override
- public Cancellable flushCacheEntriesInvalidatedByRequest(
- final HttpHost host, final HttpRequest request, final
FutureCallback<Boolean> callback) {
+ private void evictAll(final HttpCacheEntry root, final String rootKey) {
if (LOG.isDebugEnabled()) {
- LOG.debug("Flush cache entries invalidated by request: {}; {}",
host, new RequestLine(request));
+ LOG.debug("Evicting root cache entry {}", rootKey);
+ }
+ evictEntry(rootKey);
+ if (root.isVariantRoot()) {
+ for (final String variantKey : root.getVariantMap().values()) {
+ LOG.debug("Evicting variant cache entry {}", variantKey);
Review Comment:
Consider wrapping all debug log statements with a ´if
(LOG.isDebugEnabled())´ check for performance enhancement by avoiding
unnecessary string concatenation when debug logging is not enabled
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpCache.java:
##########
@@ -313,36 +324,64 @@ public CacheHit storeReusing(
return store(request, originResponse, requestSent, responseReceived,
rootKey, hit.entry);
}
- @Override
- public void flushCacheEntriesFor(final HttpHost host, final HttpRequest
request) {
- final String rootKey = cacheKeyGenerator.generateKey(host, request);
+ private void evictAll(final HttpCacheEntry root, final String rootKey) {
if (LOG.isDebugEnabled()) {
- LOG.debug("Flush cache entries: {}", rootKey);
+ LOG.debug("Evicting root cache entry {}", rootKey);
}
- try {
- storage.removeEntry(rootKey);
- } catch (final ResourceIOException ex) {
- if (LOG.isWarnEnabled()) {
- LOG.warn("I/O error removing cache entry with key {}",
rootKey);
+ removeInternal(rootKey);
+ if (root.isVariantRoot()) {
+ for (final String variantKey : root.getVariantMap().values()) {
+ LOG.debug("Evicting variant cache entry {}", variantKey);
Review Comment:
Consider wrapping all debug log statements with a ´if
(LOG.isDebugEnabled())´ check for performance enhancement by avoiding
unnecessary string concatenation when debug logging is not enabled.
Also consider replacing the for loop that evicts variant cache entries one
by one with a new ´removeEntriesBulk´ method, which will allow us to eliminate
multiple entries at once.
```
public synchronized void removeEntriesBulk(final Collection<String> urls)
throws ResourceIOException {
entries.removeEntriesBulk(urls);
}
```
and
```
public void removeEntriesBulk(final Collection<String> keysToRemove)
{
this.entrySet().removeIf(entry ->
keysToRemove.contains(entry.getKey()));
}
```
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpCache.java:
##########
@@ -129,6 +130,16 @@ HttpCacheEntry getInternal(final String cacheKey) {
}
}
+ private void removeInternal(final String cacheKey) {
Review Comment:
Shouldn't we use the same name ('evictEntry')? After all, both are internal
operations.
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java:
##########
@@ -456,57 +457,122 @@ public Cancellable storeReusing(
return store(request, originResponse, requestSent, responseReceived,
rootKey, hit.entry, callback);
}
- @Override
- public Cancellable flushCacheEntriesFor(
- final HttpHost host, final HttpRequest request, final
FutureCallback<Boolean> callback) {
- final String rootKey = cacheKeyGenerator.generateKey(host, request);
- if (LOG.isDebugEnabled()) {
- LOG.debug("Flush cache entries: {}", rootKey);
- }
- return storage.removeEntry(rootKey, new FutureCallback<Boolean>() {
+ private void evictEntry(final String cacheKey) {
+ storage.removeEntry(cacheKey, new FutureCallback<Boolean>() {
@Override
public void completed(final Boolean result) {
- callback.completed(result);
}
@Override
public void failed(final Exception ex) {
- if (ex instanceof ResourceIOException) {
- if (LOG.isWarnEnabled()) {
- LOG.warn("I/O error removing cache entry with key {}",
rootKey);
+ if (LOG.isWarnEnabled()) {
+ if (ex instanceof ResourceIOException) {
+ LOG.warn("I/O error removing cache entry with key {}",
cacheKey);
+ } else {
+ LOG.warn("Unexpected error removing cache entry with
key {}", cacheKey, ex);
}
- callback.completed(Boolean.TRUE);
- } else {
- callback.failed(ex);
}
}
@Override
public void cancelled() {
- callback.cancelled();
}
});
}
- @Override
- public Cancellable flushCacheEntriesInvalidatedByRequest(
- final HttpHost host, final HttpRequest request, final
FutureCallback<Boolean> callback) {
+ private void evictAll(final HttpCacheEntry root, final String rootKey) {
if (LOG.isDebugEnabled()) {
- LOG.debug("Flush cache entries invalidated by request: {}; {}",
host, new RequestLine(request));
+ LOG.debug("Evicting root cache entry {}", rootKey);
+ }
+ evictEntry(rootKey);
+ if (root.isVariantRoot()) {
+ for (final String variantKey : root.getVariantMap().values()) {
+ LOG.debug("Evicting variant cache entry {}", variantKey);
+ evictEntry(variantKey);
+ }
}
- return cacheInvalidator.flushCacheEntriesInvalidatedByRequest(host,
request, cacheKeyGenerator, storage, callback);
+ }
+
+ private Cancellable evict(final String rootKey) {
+ return storage.getEntry(rootKey, new FutureCallback<HttpCacheEntry>() {
+
+ @Override
+ public void completed(final HttpCacheEntry root) {
+ if (root != null) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Evicting root cache entry {}", rootKey);
+ }
+ evictAll(root, rootKey);
+ }
+ }
+
+ @Override
+ public void failed(final Exception ex) {
+ }
+
+ @Override
+ public void cancelled() {
+ }
+
+ });
+ }
+
+ private Cancellable evict(final String rootKey, final HttpResponse
response) {
+ return storage.getEntry(rootKey, new FutureCallback<HttpCacheEntry>() {
+
+ @Override
+ public void completed(final HttpCacheEntry root) {
+ if (root != null) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Evicting root cache entry {}", rootKey);
+ }
+ final Header existingETag =
root.getFirstHeader(HttpHeaders.ETAG);
+ final Header newETag =
response.getFirstHeader(HttpHeaders.ETAG);
+ if (existingETag != null && newETag != null &&
+ !Objects.equals(existingETag.getValue(),
newETag.getValue()) &&
+ !DateSupport.isBefore(response, root,
HttpHeaders.DATE)) {
+ evictAll(root, rootKey);
+ }
+ }
+ }
+
+ @Override
+ public void failed(final Exception ex) {
+ }
+
+ @Override
+ public void cancelled() {
+ }
+
+ });
}
@Override
- public Cancellable flushCacheEntriesInvalidatedByExchange(
+ public Cancellable evictInvalidated(
final HttpHost host, final HttpRequest request, final HttpResponse
response, final FutureCallback<Boolean> callback) {
if (LOG.isDebugEnabled()) {
LOG.debug("Flush cache entries invalidated by exchange: {}; {} ->
{}", host, new RequestLine(request), new StatusLine(response));
}
- if (!Method.isSafe(request.getMethod())) {
- return
cacheInvalidator.flushCacheEntriesInvalidatedByExchange(host, request,
response, cacheKeyGenerator, storage, callback);
+ final int status = response.getCode();
+ if (status >= HttpStatus.SC_SUCCESS && status <
HttpStatus.SC_CLIENT_ERROR &&
+ !Method.isSafe(request.getMethod())) {
Review Comment:
Given the relatively higher computational cost of Method.isSafe(), it's
suggested to move this condition to the beginning of our if-statement for more
efficient short-circuiting, which could improve overall performance.
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java:
##########
@@ -456,57 +457,122 @@ public Cancellable storeReusing(
return store(request, originResponse, requestSent, responseReceived,
rootKey, hit.entry, callback);
}
- @Override
- public Cancellable flushCacheEntriesFor(
- final HttpHost host, final HttpRequest request, final
FutureCallback<Boolean> callback) {
- final String rootKey = cacheKeyGenerator.generateKey(host, request);
- if (LOG.isDebugEnabled()) {
- LOG.debug("Flush cache entries: {}", rootKey);
- }
- return storage.removeEntry(rootKey, new FutureCallback<Boolean>() {
+ private void evictEntry(final String cacheKey) {
+ storage.removeEntry(cacheKey, new FutureCallback<Boolean>() {
@Override
public void completed(final Boolean result) {
- callback.completed(result);
}
@Override
public void failed(final Exception ex) {
- if (ex instanceof ResourceIOException) {
- if (LOG.isWarnEnabled()) {
- LOG.warn("I/O error removing cache entry with key {}",
rootKey);
+ if (LOG.isWarnEnabled()) {
+ if (ex instanceof ResourceIOException) {
+ LOG.warn("I/O error removing cache entry with key {}",
cacheKey);
+ } else {
+ LOG.warn("Unexpected error removing cache entry with
key {}", cacheKey, ex);
}
- callback.completed(Boolean.TRUE);
- } else {
- callback.failed(ex);
}
}
@Override
public void cancelled() {
- callback.cancelled();
}
});
}
- @Override
- public Cancellable flushCacheEntriesInvalidatedByRequest(
- final HttpHost host, final HttpRequest request, final
FutureCallback<Boolean> callback) {
+ private void evictAll(final HttpCacheEntry root, final String rootKey) {
if (LOG.isDebugEnabled()) {
- LOG.debug("Flush cache entries invalidated by request: {}; {}",
host, new RequestLine(request));
+ LOG.debug("Evicting root cache entry {}", rootKey);
+ }
+ evictEntry(rootKey);
+ if (root.isVariantRoot()) {
+ for (final String variantKey : root.getVariantMap().values()) {
+ LOG.debug("Evicting variant cache entry {}", variantKey);
+ evictEntry(variantKey);
+ }
}
- return cacheInvalidator.flushCacheEntriesInvalidatedByRequest(host,
request, cacheKeyGenerator, storage, callback);
+ }
+
+ private Cancellable evict(final String rootKey) {
+ return storage.getEntry(rootKey, new FutureCallback<HttpCacheEntry>() {
+
+ @Override
+ public void completed(final HttpCacheEntry root) {
+ if (root != null) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Evicting root cache entry {}", rootKey);
+ }
+ evictAll(root, rootKey);
+ }
+ }
+
+ @Override
+ public void failed(final Exception ex) {
+ }
+
+ @Override
+ public void cancelled() {
+ }
+
+ });
+ }
+
+ private Cancellable evict(final String rootKey, final HttpResponse
response) {
+ return storage.getEntry(rootKey, new FutureCallback<HttpCacheEntry>() {
+
+ @Override
+ public void completed(final HttpCacheEntry root) {
+ if (root != null) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Evicting root cache entry {}", rootKey);
+ }
+ final Header existingETag =
root.getFirstHeader(HttpHeaders.ETAG);
+ final Header newETag =
response.getFirstHeader(HttpHeaders.ETAG);
+ if (existingETag != null && newETag != null &&
+ !Objects.equals(existingETag.getValue(),
newETag.getValue()) &&
+ !DateSupport.isBefore(response, root,
HttpHeaders.DATE)) {
+ evictAll(root, rootKey);
+ }
+ }
+ }
+
+ @Override
+ public void failed(final Exception ex) {
+ }
+
+ @Override
+ public void cancelled() {
+ }
+
+ });
}
@Override
- public Cancellable flushCacheEntriesInvalidatedByExchange(
+ public Cancellable evictInvalidated(
Review Comment:
What about renaming to ´evictInvalidatedCacheEntries´ for enhanced
readability and better clarity of the method's operation on cache entries.
This makes it clear that the method is operating on cache entries specifically,
not just any arbitrary objects or data.
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java:
##########
@@ -456,57 +457,122 @@ public Cancellable storeReusing(
return store(request, originResponse, requestSent, responseReceived,
rootKey, hit.entry, callback);
}
- @Override
- public Cancellable flushCacheEntriesFor(
- final HttpHost host, final HttpRequest request, final
FutureCallback<Boolean> callback) {
- final String rootKey = cacheKeyGenerator.generateKey(host, request);
- if (LOG.isDebugEnabled()) {
- LOG.debug("Flush cache entries: {}", rootKey);
- }
- return storage.removeEntry(rootKey, new FutureCallback<Boolean>() {
+ private void evictEntry(final String cacheKey) {
+ storage.removeEntry(cacheKey, new FutureCallback<Boolean>() {
@Override
public void completed(final Boolean result) {
- callback.completed(result);
}
@Override
public void failed(final Exception ex) {
- if (ex instanceof ResourceIOException) {
- if (LOG.isWarnEnabled()) {
- LOG.warn("I/O error removing cache entry with key {}",
rootKey);
+ if (LOG.isWarnEnabled()) {
+ if (ex instanceof ResourceIOException) {
+ LOG.warn("I/O error removing cache entry with key {}",
cacheKey);
+ } else {
+ LOG.warn("Unexpected error removing cache entry with
key {}", cacheKey, ex);
}
- callback.completed(Boolean.TRUE);
- } else {
- callback.failed(ex);
}
}
@Override
public void cancelled() {
- callback.cancelled();
}
});
}
- @Override
- public Cancellable flushCacheEntriesInvalidatedByRequest(
- final HttpHost host, final HttpRequest request, final
FutureCallback<Boolean> callback) {
+ private void evictAll(final HttpCacheEntry root, final String rootKey) {
if (LOG.isDebugEnabled()) {
- LOG.debug("Flush cache entries invalidated by request: {}; {}",
host, new RequestLine(request));
+ LOG.debug("Evicting root cache entry {}", rootKey);
+ }
+ evictEntry(rootKey);
+ if (root.isVariantRoot()) {
+ for (final String variantKey : root.getVariantMap().values()) {
+ LOG.debug("Evicting variant cache entry {}", variantKey);
+ evictEntry(variantKey);
+ }
}
- return cacheInvalidator.flushCacheEntriesInvalidatedByRequest(host,
request, cacheKeyGenerator, storage, callback);
+ }
+
+ private Cancellable evict(final String rootKey) {
+ return storage.getEntry(rootKey, new FutureCallback<HttpCacheEntry>() {
+
+ @Override
+ public void completed(final HttpCacheEntry root) {
+ if (root != null) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Evicting root cache entry {}", rootKey);
+ }
+ evictAll(root, rootKey);
+ }
+ }
+
+ @Override
+ public void failed(final Exception ex) {
+ }
+
+ @Override
+ public void cancelled() {
+ }
+
+ });
+ }
+
+ private Cancellable evict(final String rootKey, final HttpResponse
response) {
+ return storage.getEntry(rootKey, new FutureCallback<HttpCacheEntry>() {
+
+ @Override
+ public void completed(final HttpCacheEntry root) {
+ if (root != null) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Evicting root cache entry {}", rootKey);
+ }
+ final Header existingETag =
root.getFirstHeader(HttpHeaders.ETAG);
+ final Header newETag =
response.getFirstHeader(HttpHeaders.ETAG);
+ if (existingETag != null && newETag != null &&
+ !Objects.equals(existingETag.getValue(),
newETag.getValue()) &&
+ !DateSupport.isBefore(response, root,
HttpHeaders.DATE)) {
+ evictAll(root, rootKey);
+ }
+ }
+ }
+
+ @Override
+ public void failed(final Exception ex) {
+ }
+
+ @Override
+ public void cancelled() {
+ }
+
+ });
}
@Override
- public Cancellable flushCacheEntriesInvalidatedByExchange(
+ public Cancellable evictInvalidated(
Review Comment:
What about renaming to ´evictInvalidatedCacheEntries´ for enhanced
readability and better clarity of the method's operation on cache entries.
This makes it clear that the method is operating on cache entries specifically,
not just any arbitrary objects or data.
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpCache.java:
##########
@@ -313,36 +324,64 @@ public CacheHit storeReusing(
return store(request, originResponse, requestSent, responseReceived,
rootKey, hit.entry);
}
- @Override
- public void flushCacheEntriesFor(final HttpHost host, final HttpRequest
request) {
- final String rootKey = cacheKeyGenerator.generateKey(host, request);
+ private void evictAll(final HttpCacheEntry root, final String rootKey) {
if (LOG.isDebugEnabled()) {
- LOG.debug("Flush cache entries: {}", rootKey);
+ LOG.debug("Evicting root cache entry {}", rootKey);
}
- try {
- storage.removeEntry(rootKey);
- } catch (final ResourceIOException ex) {
- if (LOG.isWarnEnabled()) {
- LOG.warn("I/O error removing cache entry with key {}",
rootKey);
+ removeInternal(rootKey);
+ if (root.isVariantRoot()) {
+ for (final String variantKey : root.getVariantMap().values()) {
+ LOG.debug("Evicting variant cache entry {}", variantKey);
Review Comment:
Consider wrapping all debug log statements with a ´if
(LOG.isDebugEnabled())´ check for performance enhancement by avoiding
unnecessary string concatenation when debug logging is not enabled.
Also consider replacing the for loop that evicts variant cache entries one
by one with a new ´removeEntriesBulk´ method, which will allow us to eliminate
multiple entries at once.
```
public synchronized void removeEntriesBulk(final Collection<String> urls)
throws ResourceIOException {
entries.removeEntriesBulk(urls);
}
```
and
```
public void removeEntriesBulk(final Collection<String> keysToRemove)
{
this.entrySet().removeIf(entry ->
keysToRemove.contains(entry.getKey()));
}
```
##########
httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/BasicHttpAsyncCache.java:
##########
@@ -456,57 +457,122 @@ public Cancellable storeReusing(
return store(request, originResponse, requestSent, responseReceived,
rootKey, hit.entry, callback);
}
- @Override
- public Cancellable flushCacheEntriesFor(
- final HttpHost host, final HttpRequest request, final
FutureCallback<Boolean> callback) {
- final String rootKey = cacheKeyGenerator.generateKey(host, request);
- if (LOG.isDebugEnabled()) {
- LOG.debug("Flush cache entries: {}", rootKey);
- }
- return storage.removeEntry(rootKey, new FutureCallback<Boolean>() {
+ private void evictEntry(final String cacheKey) {
+ storage.removeEntry(cacheKey, new FutureCallback<Boolean>() {
@Override
public void completed(final Boolean result) {
- callback.completed(result);
}
@Override
public void failed(final Exception ex) {
- if (ex instanceof ResourceIOException) {
- if (LOG.isWarnEnabled()) {
- LOG.warn("I/O error removing cache entry with key {}",
rootKey);
+ if (LOG.isWarnEnabled()) {
+ if (ex instanceof ResourceIOException) {
+ LOG.warn("I/O error removing cache entry with key {}",
cacheKey);
+ } else {
+ LOG.warn("Unexpected error removing cache entry with
key {}", cacheKey, ex);
}
- callback.completed(Boolean.TRUE);
- } else {
- callback.failed(ex);
}
}
@Override
public void cancelled() {
- callback.cancelled();
}
});
}
- @Override
- public Cancellable flushCacheEntriesInvalidatedByRequest(
- final HttpHost host, final HttpRequest request, final
FutureCallback<Boolean> callback) {
+ private void evictAll(final HttpCacheEntry root, final String rootKey) {
if (LOG.isDebugEnabled()) {
- LOG.debug("Flush cache entries invalidated by request: {}; {}",
host, new RequestLine(request));
+ LOG.debug("Evicting root cache entry {}", rootKey);
+ }
+ evictEntry(rootKey);
+ if (root.isVariantRoot()) {
+ for (final String variantKey : root.getVariantMap().values()) {
+ LOG.debug("Evicting variant cache entry {}", variantKey);
Review Comment:
Consider wrapping all debug log statements with a ´if
(LOG.isDebugEnabled())´ check for performance enhancement by avoiding
unnecessary string concatenation when debug logging is not enabled
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
