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 for redis cachedb backend #2

Closed
wants to merge 8 commits into from

Conversation

jinmeiib
Copy link
Contributor

@jinmeiib jinmeiib commented Mar 8, 2018

This change adds to unbound support for a new cached backend that uses a Redis server as the storage.

This implementation depends on the hiredis client library (https://redislabs.com/lp/hiredis/). And unbound should be built with both --enable-cachedb and --with-libhiredis[=PATH] (where $PATH/include/hiredis/hiredis.h should exist).

I've locally confirmed the following:

  • build succeeds with or without --with-libhiredis on Linux (Ubuntu 16.04.5), MacOS X 10.11.6 and FreeBSD 10.3.
  • 'make test' passed (on Linux)
  • 'make longtest' passed (on Linux)

@chantra
Copy link

chantra commented Mar 10, 2018

Thanks @jinmeiib ! that's great. I am really happy to see this getting in.
Would it make sense to try to keep the redis code in separate files (as much as possible) for the sake of separating implementation in their own files?

@jinmeiib
Copy link
Contributor Author

Thanks for the comment. Yes, I think that makes sense especially if we expect more backend will be added. I don't remember why I chose the single-file approach at that point, but perhaps it's because the size of redis-specific code isn't expected to be too big and I was not sure if we really have more backends and the speparation would be premature generalization. Anyway, I'm now asking NLNet Labs people for a review. If they want me to separate it, I'm happy to do that. If they want to make it by themselves and merge it, that's also fine. If they want to keep the current style for now, that's also fine for me.

@wcawijngaards
Copy link
Member

wcawijngaards commented Mar 15, 2018 via email

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.

3 participants