ktheory / dalli-elasticache

A wrapper for Dalli with support for AWS ElastiCache
MIT License
128 stars 32 forks source link

Raise exception if endpoint is wrong format #19

Closed yozlet closed 2 years ago

yozlet commented 10 years ago

So @cpapazian and I were bitten by forgetting to include the colon and port, and when this happens the resulting error (a failure in remote_command()) takes a while to track down.

We initially wrote this as just defaulting the port to 11211 if unspecified. However, this severely increases the pain of debugging connections to non-standard ports with only a very minor usability improvement for everyone else.

zmillman commented 10 years ago

I like this! :+1:

ktheory commented 10 years ago

Seems harsh to raise an exception when you omit the port, since you probably want 11211.

Also, not sure why we need ENDPOINT_REGEX.

How about changing the initialize method to default the port to 11211?

def initialize(endpoint)
  @host, port = endpoint.split(':')
  @port = (port || 11211).to_i
end
cpapazian commented 10 years ago

@zmillman i've updated this to use if/else instead of or.

cpapazian commented 10 years ago

@ktheory our original approach was to default to 11211, like you suggest, but we thought this was simpler. FWIW, I kind of like the ENDPOINT_REGEX, as it does some sanity checking, but split works, too. In either case, adding the raise if either split or match fails seems to be a good thing ... so if you guys think it's a good idea, we can go ahead and add some logic to default the port to this PR. Thanks!

petergoldstein commented 2 years ago

Thanks for submitting this.

We're addressing the underlying issues using some different code (https://github.com/ktheory/dalli-elasticache/pull/37) so I'm closing this PR.