378 persisted disk cache for cardinality queries #379
Conversation
...hefquin/engine/federation/access/impl/FederationAccessManagerWithPersistedDiskCacheTest.java
Outdated
Show resolved
Hide resolved
...hefquin/engine/federation/access/impl/FederationAccessManagerWithPersistedDiskCacheTest.java
Outdated
Show resolved
Hide resolved
...gine/src/main/java/se/liu/ida/hefquin/engine/federation/access/impl/CardinalityCacheMap.java
Outdated
Show resolved
Hide resolved
...ida/hefquin/engine/federation/access/impl/FederationAccessManagerWithPersistedDiskCache.java
Outdated
Show resolved
Hide resolved
...gine/src/main/java/se/liu/ida/hefquin/engine/federation/access/impl/CardinalityCacheMap.java
Outdated
Show resolved
Hide resolved
...gine/src/main/java/se/liu/ida/hefquin/engine/federation/access/impl/CardinalityCacheKey.java
Outdated
Show resolved
Hide resolved
…ion/access/impl/FederationAccessManagerWithPersistedDiskCacheTest.java Co-authored-by: Olaf Hartig <[email protected]>
…ion/access/impl/FederationAccessManagerWithPersistedDiskCacheTest.java Co-authored-by: Olaf Hartig <[email protected]>
…ion/access/impl/CardinalityCacheMap.java Co-authored-by: Olaf Hartig <[email protected]>
…ion/access/impl/CardinalityCacheKey.java Co-authored-by: Olaf Hartig <[email protected]>
|
The requested changes have been addressed. @hartig, could you check if looks okay? |
| * | ||
| * @param entries a map of IDs to objects to be added to the cache | ||
| */ | ||
| void putAll( Map<IdType, ObjectType> map ); |
There was a problem hiding this comment.
I don't understand the purpose of adding this method in this particular interface. In fact, this method doesn't even seem to be used anywhere.
Shouldn't save() and load() be methods of this interface?
There was a problem hiding this comment.
Indeed, putAll is no longer used. Adding save and load to the interface makes sense.
| * | ||
| * @param <K> The key type for caching cardinality responses. | ||
| */ | ||
| public class CardinalityCacheMap<K> implements PersistableCache<K, CompletableFuture<CardinalityResponse>> { |
There was a problem hiding this comment.
I would rename this class to PersistableCardinalityCacheImpl.
There was a problem hiding this comment.
Yes, this fits better with the naming scheme.
| public CompletableFuture<CardinalityResponse> issueCardinalityRequest( final SPARQLRequest req, | ||
| final SPARQLEndpoint fm ) |
There was a problem hiding this comment.
Shouldn't the other issueCardinalityRequest methods have an implementation in this class as well?
There was a problem hiding this comment.
Yes, they should. I'll add them and the corresponding tests.
There was a problem hiding this comment.
Is this ready for me to look at again?
There was a problem hiding this comment.
Not yet, but I will let you know when it's ready. Working on it now.
No description provided.