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

Redis connection parameters do not match those of redis-rb #13

Open
aaaronic opened this issue Apr 25, 2016 · 4 comments
Open

Redis connection parameters do not match those of redis-rb #13

aaaronic opened this issue Apr 25, 2016 · 4 comments

Comments

@aaaronic
Copy link

The redis gem accepts slightly different options than faye-redis.

For example, faye-redis expects a :database option, whereas the same option in the redis gem is called :db. Additionally, faye-redis doesn't support options keys as strings, whereas the redis gem does (this could be more intentional, admittedly, but when loading from a YAML config keys come in as strings).

This is particularly notable, because when faye-redis can't connect successfully to Redis, handshake requests just time out, instead of logging anything about not being able to connect to Redis (lost a day of development before noticing the redis-rb vs faye-redis options differences).

aaaronic added a commit to aaaronic/faye-redis-ruby that referenced this issue Apr 25, 2016
This brings the connection options more inline with redis-rb

[closes faye#13]
@jcoglan
Copy link
Collaborator

jcoglan commented Apr 27, 2016

Sorry, I'm not sure I follow. I'm not sure why it's important that faye-redis takes the same options as redis-rb, especially given it does not depend on that gem.

I'd also much rather not allow strings for option names; they are symbols with special meaning to the program, as opposed to arbitrary textual data. You can use symbols as keys in YAML if you want to load config directly from a file and pass it to this gem.

@aaaronic
Copy link
Author

aaaronic commented May 2, 2016

Our project would rather have one YAML file for all redis configuration. We're using that config for the redis gem as well as faye-redis. In trying to troubleshoot why faye was timing out on handshake requests after switching to redis, it was pretty confusing that the redis gem was connecting just fine from inside the same block of code using the same configuration.

Since it's pretty low-hanging to accommodate, I assumed allowing for the same parameters should not be much of an issue. Supporting string-based argument names isn't exactly arbitrary. Symbols are no less arbitrary, and supporting both takes almost no effort.

Once I finally figured out the issue, I had my app munge the config from our YAML file to match your expectations ("db" => "database", then symbolize_keys on the hash), but it would be nice not to resort to such hackery.

@timcraft
Copy link

timcraft commented May 3, 2016

@aaaronic What about using a bit more YAML? Something like this:

:common: &common
  :host: example.com
  :port: 1234

:redis_config:
  <<: *common
  :db: 15

:faye_redis_config:
  <<: *common
  :database: 15

No need for symbolize_keys hackery or changing faye-redis options.

@aaaronic
Copy link
Author

aaaronic commented May 3, 2016

I have an environment-based configuration, with development, test, and production as the current top-level keys (a staging/release environment is also likely soon). I'd rather not have more than the three top-level keys to support a different format of keys for faye_redis, separate from my typical redis settings.

The symbols as keys feature doesn't seem to be part of YAML proper, it just seems to be something the Ruby implementation offers (other languages pull those keys as ":common", ":redis_config", etc.), so I'm not enthusiastic about relying on it.

If you're completely uninterested in supporting strings as arguments, feel free to just close the issue/PR. Debating it further is probably not going to prove fruitful. I can live with the hacks in my faye.ru indefinitely, if need be.

I might find time at some point to submit a separate PR addressing the other half of the scenario that was giving me trouble: when faye-redis can't connect to the redis db for the config provided, handshake requests just time out -- there's never a logged error about not being able to connect.

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

No branches or pull requests

3 participants