ktheory / dalli-elasticache

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

Does not work with Google Cloud MemoryStore #32

Closed mheffner closed 2 years ago

mheffner commented 3 years ago

GCP MemoryStore memcached also supports this discovery command: https://cloud.google.com/memorystore/docs/memcached/using-auto-discovery

However, when you run the STATS command you get back the following:

STAT version UNKNOWN

So this version check won't work: https://github.com/ktheory/dalli-elasticache/blob/2224e4c862962d692fd115b07bed613eae0aeb5b/lib/dalli/elasticache/auto_discovery/endpoint.rb#L52.

I hardcoded the check to default to the new version and after that it worked to discover the servers.

petergoldstein commented 2 years ago

@mheffner So what version of the memcached interface does GCP MemoryStore support? Is that exposed or documented anywhere?

I do think this is probably far less of an issue than it was ~5 years ago, as pretty much everyone runs on a Memcached version that is 1.4.14 or more at this point.

mheffner commented 2 years ago

@petergoldstein

They currently support version 1.5.16.

I do think this is probably far less of an issue than it was ~5 years ago, as pretty much everyone runs on a Memcached version that is 1.4.14 or more at this point.

:+1:

petergoldstein commented 2 years ago

@mheffner I'm planning to put out an updated version next week. I'd like to include a fix for this issue so we can expand the scope of the gem to Google Cloud MemoryStore. Do you have an environment where you could verify this before release?

mheffner commented 2 years ago

@petergoldstein Yes, I could test out a pre-release.

petergoldstein commented 2 years ago

@mheffner Great. I should have something Monday or Tuesday.

petergoldstein commented 2 years ago

@mheffner One additional request. Any chance you could get me the raw output of a config get cluster run against a Google Cloud Memory Store? I want to make sure there aren't any divergences that would cause trouble for the regexp matching.

If you could I'd appreciate it. You can just add the response to this issue.

mheffner commented 2 years ago

@petergoldstein yeah, here you go:

config get cluster
CONFIG cluster 0 58
1
10.1.1.2|10.1.1.2|11211 10.1.1.3|10.1.1.3|11211

END
petergoldstein commented 2 years ago

@mheffner Not quite pre-release yet, but this branch - https://github.com/ktheory/dalli-elasticache/pull/37 - should work for Google Cloud Memory Store

petergoldstein commented 2 years ago

Ok @mheffner , now I'd call that PR pre-release. If we can confirm Google Cloud MemoryStore I'll probably make a few README changes, but otherwise I think that PR is basically what I'd like to see go out as a 1.0.0. I'm aiming for Tuesday or Wednesday of next week.

mheffner commented 2 years ago

@petergoldstein That PR did not appear to work. I'm just guessing, but I think where you do:

 def legacy_config?
          return false unless engine_version
          return false if engine_version == 'unknown'

It should be:

          return false if engine_version.downcase == 'unknown'

given that the version string from GCP is:

STAT version UNKNOWN
petergoldstein commented 2 years ago

@mheffner Can you give it another shot? I just pushed an update.

mheffner commented 2 years ago

@petergoldstein looks good AFAICT :+1:

petergoldstein commented 2 years ago

Thanks @mheffner!

petergoldstein commented 2 years ago

Thanks for filing the issue. The fix for this is included in dalli-elasticache version 1.0.0 which was just published. Closing. Please reopen if you have any problems with that version.