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

allow redis to be configured with sentinels #14

Closed
wants to merge 1 commit into from
Closed

allow redis to be configured with sentinels #14

wants to merge 1 commit into from

Conversation

mwmessin
Copy link

@mwmessin mwmessin commented Sep 2, 2015

  • add a -j config option to the daemon to configure with a json blob (for both redis and the apps list)
  • add sentinels config to redis

@tompesman
Copy link
Member

Thank you for your PR! There are a few issues with this PR.

  • lib/pushr/configuration: why change the all method to apps?
  • lib/pushr/core: why ignore the default values in the options hash?
  • lib/pushr/daemon: scale_redis_connections method is changed to inject a json configuration
  • lib/pushr/redis_connection: existing behaviour is removed
  • lib/pushr/version: this is the responsibility of the gem maintainer

I think we should split some of the stuff you want to get into this gem.

  • JSON config file is just for the redis configuration, or is it also to include the configurations?
  • add support for sentinels

Agree?

@mwmessin
Copy link
Author

mwmessin commented Sep 3, 2015

  • all was changed to apps because I wanted one config file for all things. So I added a new top level of keys to the config hash of redis: {the redis config}, apps: [the app array config]. read_from_json then only returns the 'apps' key
  • no good reason to ignore default values in the options hash
  • scale_redis_connections seems to be the final redis client create for the daemon, so it needed the sentinel config from the json. The general problem is that the structure of the json config is not the same as the existing yml config + cmd line redis config. We have a strong desire to have all our config in one place and not in volatile storage (redis).
  • redis_connection existing behaviour wasn't used by us, could put it back.
  • versioning is maintained internally for our fork.

You would like 2 PRs for each feature?

@tilo
Copy link

tilo commented Sep 24, 2015

sweet! :)

This pull request was closed.
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