faye / faye-redis-ruby

Redis engine backend for Faye
80 stars 38 forks source link

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

Open aaaronic opened 8 years ago

aaaronic commented 8 years ago

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).

jcoglan commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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.