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

HTTPCLIENT-2284: Cache entry representation improvements #477

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

ok2c
Copy link
Member

@ok2c 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

@ok2c
Copy link
Member Author

ok2c commented Aug 21, 2023

@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.
Please review.

Copy link
Member

@arturobernalg arturobernalg left a 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
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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
@arturobernalg arturobernalg merged commit b80426c into apache:5.4.x Aug 22, 2023
7 checks passed
ok2c added a commit that referenced this pull request Dec 13, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants