adding TTL option for redis cache adapter #3397
Conversation
|
@f0ster this looks good to me. would you be willing to write a unit test that proves that the default constructor value works as expected? |
|
@f0ster updated the pull request - view changes |
|
@acinader I will take a look, I didn't see any unit tests for the other cache adapters testing parameters so I didn't include.. |
|
@f0ster updated the pull request - view changes |
|
@f0ster updated the pull request - view changes |
|
@acinader I've added a spec/behaviour test for redis TTL https://github.com/f0ster/parse-server/blob/6a5c0dd09b460eddac7bb8fdc75b24dd46532119/spec/RedisCacheAdapter.spec.js |
|
@f0ster updated the pull request - view changes |
| @@ -1,17 +1,18 @@ | |||
| import redis from 'redis'; | |||
| import logger from '../../logger'; | |||
|
|
|||
| const DEFAULT_REDIS_TTL = 30 * 1000; // 30 seconds in milliseconds | |||
| const DEFAULT_REDIS_TTL = 30; // 30 seconds in milliseconds | |||
There was a problem hiding this comment.
@f0ster why the change here?
we use psetex https://redis.io/commands/psetex
so if i've got it right, this change is not good? cause we'll just cache for 30 millis by default.
PS sorry for the delay.
spec/RedisCacheAdapter.spec.js
Outdated
| cache.put(KEY, VALUE) | ||
| .then(() => cache.get(KEY)) | ||
| .then((value) => expect(value).toEqual(VALUE)) | ||
| .then(wait.bind(null, 50)) |
|
|
||
| cache.put(KEY, VALUE) | ||
| .then(() => cache.get(KEY)) | ||
| .then((value) => expect(value).toEqual(VALUE)) |
There was a problem hiding this comment.
what!@?! mind blown. I don't even understand how returning expect.... is "thenable", but super cool idiom. going to start using....
|
@f0ster super nice tests. thanks. just one outstanding issue on seconds v millis. |
|
@f0ster updated the pull request - view changes |
facebook-github-bot
added
GH Review: review-needed
and removed
GH Review: needs-revision
labels
|
@acinader I've gone ahead and updated the default ttl to use ms |
make timeout values really really small so our test run fast :).
Fix the redis cache spec to construct the cache with the anticipated ttl
|
@f0ster updated the pull request - view changes |
|
Grrrr, not sure why that test is failing. Obviously it works locally and may just be that the times are too short! And it passed once and failed once :( In any event I'd like to play with it a bit to see if I can reproduce, understand and fix, but it'll take a few days before I can look at it. |
|
So I couldn't reproduce the problem and when i re-ran the test it worked. So I am going to merge this now and i'll watch the tests over the next few days. if this sucker keeps failing i'll increase the tolerances. |
|
@acinader Awesome, thank you. I was also unable to reproduce, perhaps it was some sort of race condition ? |
No description provided.