Skip to content

Cache and serve 404 response#360

Merged
eduardoboucas merged 12 commits intodevelopfrom
feature/cache-404-response
May 16, 2018
Merged

Cache and serve 404 response#360
eduardoboucas merged 12 commits intodevelopfrom
feature/cache-404-response

Conversation

@jimlambie
Copy link
Copy Markdown
Contributor

No description provided.

@jimlambie jimlambie requested a review from eduardoboucas May 14, 2018 12:39
Comment thread dadi/lib/cache/index.js Outdated

return cache.get(encryptedKey).catch(err => { // eslint-disable-line handle-callback-err
return null
return cache.get(encryptedKey).then(stream => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I'm a fan of changing the response format of this method to accommodate the need to return multiple values when requesting metadata. My preference would be to have a getMetadata method in the cache module, which would be called separately after an item has been successfully obtained from cache.

Instead of:

  1. Call cache.getStream(k), which returns an object x, and access the stream via x.cachedStream and the metadata via x.metadata

I would do:

  1. Call cache.getStream(k) which returns a stream or null
  2. If the above is truthy, I make a call for its metadata if I'm interested in it – cache.getMetadata(k)

This would mean no breaking changes to existing calls to getStream, and also you wouldn't need to drill down into the response object if you were only interested in the stream and not any metadata associated to it.

@jimlambie jimlambie requested a review from eduardoboucas May 16, 2018 05:10
@jimlambie
Copy link
Copy Markdown
Contributor Author

This has been load tested

@eduardoboucas eduardoboucas merged commit 67e7302 into develop May 16, 2018
@jimlambie jimlambie deleted the feature/cache-404-response branch June 6, 2018 08:55
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