Handle HTTP calls asynchronously, add response caching #163
Conversation
| return null | ||
| } | ||
|
|
||
| let backend = response.data.wiki_queryservice_namespace.backend |
There was a problem hiding this comment.
We could also cache this computation. I steered away from it as it would mean even more noise in this changeset. Any opinions?
There was a problem hiding this comment.
I think this is probably the right call. IMHO if we start trying to optimise this computation we might also want to use native URL parsing etc. https://nodejs.org/api/url.html before adding the complexity of more caching.
| // Try and log on failures (so this process keeps running...) | ||
| try { | ||
| var responseCache = new LRUCache({ | ||
| ttl: parseInt(process.env.RESPONSE_CACHE_TTL || 60 * 60, 10) * 1000 |
There was a problem hiding this comment.
This needs a chart update. Any opinions on how long to cache such an item? I'd think 60 minutes is a good value to have.
There was a problem hiding this comment.
PR here wbstack/charts#118
There was a problem hiding this comment.
60mins seems fine to me. These mappings are currently very stable. We have never changed them so from historical data these could be cache indefinitely but 60mins means that we shan't have to worry too much if for some reason we do change them (e.g. if in future we allow reusing domains)
|
Seems returning a Promise from a resolver is expected and would work just like the synchronous version before https://github.com/OptimalBits/redbird/blob/cb7781e9c471bcb497ea4af1ddabf4fdf4432caa/lib/proxy.js#L578 |
Ticket https://phabricator.wikimedia.org/T339906
Having asynchronous resolvers in redbird is not documented, but is done in the tests: https://github.com/OptimalBits/redbird/blob/cb7781e9c471bcb497ea4af1ddabf4fdf4432caa/test/test_custom_resolver.js#L225-L233 so I would assume this works as expected.
I tried to follow the existing behavior as closely as possible, which is why the code is still a bit quirky in places. For review, it might help to look at the commits which isolate the two changes munged together into this PR.
I was able to test these changes successfully in my local env.