[Argus] Improves 'HEAD /assets/{id}' requests latency by implementing caching QN requests#4834
Conversation
mnaamani
left a comment
There was a problem hiding this comment.
Implementation looks good. I left just a clarification question.
I have to say I was surprised that there is no builtin cache expiry config, and I presume that is why you had to add way to track the age of cached object.
Lets make the max age configurable in config.yml (or do you think it makes more sense to hardcode it?) and bump the distributor node version.
| return defaultDataIdFromObject(object) | ||
| typePolicies: { | ||
| ProcessorState: { | ||
| keyFields: [], |
There was a problem hiding this comment.
Does this tell apollo client explicitly NOT to cache processor state ?
There was a problem hiding this comment.
Thank you for pointing this out. This is actually incorrect, basically, this is just the alternative & newer way (introduced in Apollo 3.0) to assign the cache IDs to objects. We used dataIdFromObject previously. I have fixed it
| private apolloClient: ApolloClient<NormalizedCacheObject> | ||
| private logger: Logger | ||
| private CachedObjectsAge: Map<string, Date> = new Map() | ||
| private CACHED_OBJECT_MAX_AGE = 1000 * 60 // 1 min |
There was a problem hiding this comment.
1min sounds good, but we probably want to make this a configurable in the config.yml file
| ): FragmentT | null { | ||
| // Get the fragment name, usually first element of the definitions array is the name of the top level fragment | ||
| const fragmentName = (fragment.definitions[0] as FragmentDefinitionNode).name.value | ||
| if (!fragmentName) { |
There was a problem hiding this comment.
Do we want to also check that the fragmentName === "id"
There was a problem hiding this comment.
So I checked that Id isn't necessarily same as the fragment name. id refers to the cached object ID (e.g. for the StorageDataObject entity type it can be StorageDataObject:1) however the fragment name refers to the specific fragment of the entity type that we want to query from the cache, e.g. DataObjectDetails
@zeeshanakram3 Found a third party cache implementation for apollo client, is it something we can use to make our code a bit cleaner? https://github.com/NerdWalletOSS/apollo-cache-policies |
Thanks for sharing, will test it. |
|
|
Works as expected. // distributor-node/src/services/content/ContentService.ts
public async objectStatus(
objectId: string,
qnFetchPolicy: QueryFetchPolicy = 'no-cache'
): Promise<ObjectStatus> {
/*
const pendingDownload = this.stateCache.getPendingDownload(objectId)
if (!pendingDownload && this.exists(objectId)) {
return { type: ObjectStatusType.Available, path: this.path(objectId) }
}
if (pendingDownload) {
return { type: ObjectStatusType.PendingDownload, pendingDownload }
}
*/
const objectInfo = await this.networking.dataObjectInfo(objectId, qnFetchPolicy)
...then I tested (crudely) Tried with different TTL values also. |
mnaamani
left a comment
There was a problem hiding this comment.
Nice work.
This will also benefit GET requests.
Not clear how much impact this will have but if there is any kind of repeated requests to assets the node doesn't have this fix will cover it.
addresses #4814
This PR implements
max-agebased cache strategy forStorageDataObjectQN entity, whose corresponding data object asset is missing on the distributor-node & for which a QN request is need to be made.So whenever a
HEAD /assets/{id}request is made, it could be satisfied in the following ways:CACHED_OBJECT_MAX_AGEvalue, a network call would still be made to fetch the object and update the cache.CACHED_OBJECT_MAX_AGE, the object would be served from the cache