Add memcached server address to Set() error #2775
Conversation
Signed-off-by: Marco Pracucci <[email protected]>
pkg/cacheutil/memcached_client.go
Outdated
| @@ -463,3 +464,8 @@ func (c *memcachedClient) resolveAddrs() error { | |||
|
|
|||
| return c.selector.SetServers(servers...) | |||
| } | |||
|
|
|||
| func (c *memcachedClient) getServerAddrByKey(key string) net.Addr { | |||
| addr, _ := c.selector.PickServer(key) | |||
There was a problem hiding this comment.
could we be strict check the error? Or at least document why we are 100% confident the error does not need checking?
There was a problem hiding this comment.
IMO if this fails we just log empty addr, which is fine, no?
There was a problem hiding this comment.
IMO if this fails we just log empty addr, which is fine, no?
Correct. That was my intention. What's your take @squat ?
There was a problem hiding this comment.
that's OK but nothing is preventing anyone from using this internally and getting surprised later when something doesn't work and there's no error. Any reason this really needs to be a new func vs simply inlined above 291? Otherwise I would suggest to add this exact thing as a comment :)
There was a problem hiding this comment.
Right. I removed the function. What's about now?
Signed-off-by: Marco Pracucci <[email protected]>
* Add memcached server address to Set() error Signed-off-by: Marco Pracucci <[email protected]> * Addressed review comments Signed-off-by: Marco Pracucci <[email protected]> Signed-off-by: Harshitha1234 <[email protected]>
Changes
Today I was investigating the error
memcache: unexpected response line from \"set\": \"SERVER_ERROR Out of memory during read\\r\\n\in Cortex and I had some hard time figuring out which was the memcached server.In this PR I propose to inject the server address to the Set() error.
Note: the same thing is difficult to do with the GetMulti() because we may potentially touch many (sometimes all) memcached server during a multi get request.
Verification
Existing tests.