Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Redis backend via Readthis gem #6

Closed
wants to merge 2 commits into from

Conversation

dlackty
Copy link

@dlackty dlackty commented Feb 6, 2017

Nowadays Redis becomes go-to cache solution for newly created Rails app. It's faster and provides other useful functionalities beyond cache.

P.S. Which CI service is in use right? I saw both Circle & Travis configuration but couldn't find building messages.

@djudd
Copy link

djudd commented Feb 8, 2017

Thanks for the PR! Might not have time to test and merge for a bit but it makes sense to me--concerns from anyone else? @nate00 @alexey-dc ?

I think tests are running on Academia.edu's private CircleCI repository (not 100% sure since I no longer work there). Not sure the Travis setup was ever finished.

@dlackty
Copy link
Author

dlackty commented Feb 8, 2017

Because this is a public gem, I'd recommend using Travis CI that everyone can see building status. I could help setting up CI configuration but need your help to enable Travis CI to start building.

@nate00
Copy link
Member

nate00 commented Feb 8, 2017

This seems like a good idea to me. I'll review this more closely in the next week, and investigate the CI situation.

@dlackty could you add a mention of Readthis to the README?

@nate00 nate00 self-assigned this Feb 8, 2017
@nate00
Copy link
Member

nate00 commented Feb 11, 2017

I don't think this PR will work as-is. Dalli's increment method has an :initial option that isn't supported by Readthis, and we rely on that option. Here's the documentation of that option.

(As an aside, I wish there was some sort of duck typing for method options, so we could check for these options directly rather than requiring Dalli specifically.)

In any case, I'm gonna work on making the CI public in the next few days.

@nate00
Copy link
Member

nate00 commented Feb 11, 2017

Okay, builds are now publicly visible. (There's a clickable badge in the README.)

I think that adding Redis support will require the following steps:

  1. Determine which special features of Dalli this gem depends on.
  2. Eliminate the dependencies on any features not common to Dalli and Readthis.

I'm not actually sure whether the second one is possible – it depends what the special features are.

@dlackty if you want to take on this work, I'm happy to accept an updated PR. Otherwise I'll close this PR and open an issue.

@dlackty
Copy link
Author

dlackty commented Feb 12, 2017

@nate00 As far as I investigated, the only and main special feature provided by memcached and dalli is initial value for increment. I've checked both Readthis and Redis APIs to confirm there's no equivalent command.

To eliminate it, we can just check using regular Rails.cache.exists? and then increment only if key is presented. However, this would require 2 calls to memcached server.

So I think a better way is to

  1. Change current implementation to a standard and compatible one
  2. Create a special path when memcached & dalli detected.

How do you think?

@nate00
Copy link
Member

nate00 commented Feb 13, 2017

@dlackty that sounds exactly right to me.

I suppose separating exists? and increment introduces a race condition (if the key expires immediately after the call to exists?). Do you think that's worth solving (e.g., with a Redis script)? I don't have a strong opinion.

@dlackty
Copy link
Author

dlackty commented Mar 8, 2017

After investigation, I decided to give up this branch because there's not only :initial option but also race_condition_ttl absent in Redis.

@dlackty dlackty closed this Mar 8, 2017
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.

None yet

3 participants