Skip to content

Single redis DB for discourse#1431

Closed
yoav-steinberg wants to merge 14 commits intodiscourse:masterfrom
RedisLabs:master
Closed

Single redis DB for discourse#1431
yoav-steinberg wants to merge 14 commits intodiscourse:masterfrom
RedisLabs:master

Conversation

@yoav-steinberg
Copy link
Copy Markdown

The idea is to remove the use of multiple redis databases (db/cache_db) in discourse. It is generally considered bad design to use multiple redis databases. Instead the use of keyspace prefixes or separate redis instances is preferable.
Keyspace prefixes are good in case you want to be able to run the same app multiple times on the server. It seems like discourse does this to some extent but fails in other cases (such as no use of keyspaces for sidekiq).
Use of separate redise instances is ideal for it enables fine tuning of of data persistence, eviction policy, etc. based on the data requirements.
Ideally everything in discourse will be keyspace prefixed and you'll optionally you could use separate instances for each functionality (cache/job management/messaging/etc...).

With this patch I merely attempt to remove the requirement for multiple databases on the same instance, since it doesn't provide any added benefit (does it?) and therefore ads complexity (and yes, it makes it work with redis-cloud.com).

Let me know what you think.

@discoursebot
Copy link
Copy Markdown

Thanks for contributing this pull request! Could you please sign our CLA so we can review it? http://www.discourse.org/cla

@yoav-steinberg
Copy link
Copy Markdown
Author

Signed.

@SamSaffron
Copy link
Copy Markdown
Member

@yoav-steinberg I am fine with this change but can you

  1. super confirm this all works as expected (make sure there is a test that covers this)
  2. get rid of the merge commit.

@yoav-steinberg
Copy link
Copy Markdown
Author

@SamSaffron regarding testing. Can you point me out to how to run the test suite and where I can define a new test?

@ZogStriP
Copy link
Copy Markdown
Member

@yoav-steinberg either bundle exec rspec or bundle exec rake autospec (be sure to run bundle exec rake test:migrate before)

@SamSaffron
Copy link
Copy Markdown
Member

@yoav-steinberg this is a very high risk change, I definitely want to know you are able to run our test suite and contribute tests before pulling anything like this in.

@yoav-steinberg
Copy link
Copy Markdown
Author

Created new pull request with relevant fixes (#1464).
I guess you can chuck this.

@ZogStriP ZogStriP closed this Sep 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants