-
Notifications
You must be signed in to change notification settings - Fork 972
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
HTTPCLIENT-2284: Cache entry representation improvements #477
Conversation
ok2c
commented
Aug 21, 2023
- request URI stored in cache is now normalized and is presently expected to be the same as the root key
- removed references to variant cache keys from the cache entry
- Variants in root entries are now represented as a set, not as a map
@arturobernalg Here's what I hope is going to be the last round of changes to the HttpCacheEntry and the related classes. This change-set should also help partially resolve HTTPCLIENT-2284 by making it easier to implement custom cleanup of evicted cache entries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ok2c
The change look good. I just add a small comment.
@@ -328,7 +330,7 @@ public HttpCacheStorageEntry deserialize(final byte[] serializedObject) throws R | |||
statusLine.getStatusCode(), | |||
responseHeaders, | |||
resource, | |||
variantMap | |||
!variants.isEmpty() ? variants : null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we delegate everything to the internal constructor of HttpCacheEntry
? I think its redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we delegate everything to the internal constructor of
HttpCacheEntry
? I think its redundant.
@arturobernalg I am not sure I understand. You mean the variants.isEmpty()
check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ok2c my bad. No change need it. Sorry about that
* request URI stored in cache is now normalized and is presently expected to be the same as the root key * removed references to variant cache keys from the cache entry * Variants in root entries are now represented as a set, not as a map
* request URI stored in cache is now normalized and is presently expected to be the same as the root key * removed references to variant cache keys from the cache entry * Variants in root entries are now represented as a set, not as a map