Log memcached connection issues, handle case of no healthy servers #114
Conversation
|
@ianshward I think you are indeed on the right track here. But I do see that the tests are failing, was that caused by your patch? |
Previously, connectionIssue was only called if a response was received from the memcached service, and the response was a SERVER_ERROR. However, if the node client could not connect to a memcached server at all, this case was not handled. Now, on the error event on the stream, call connectionIssue. Additionally, before trying to connect to memcached, first check whether any healthy servers exist.
|
@3rd-Eden yes I'd broken a few tests. I fixed them and at the same time simplified the logic in the change. I also added two tests, one which tests that a healthy server is removed, and the other tests that a request which went to a failed server is rebalanced to the remaining healthy server when |
Log memcached connection issues, handle case of no healthy servers
|
Pull request looks good to me, would you be interested in getting push access @ianshward ? |
|
@3rd-Eden sure re: push access. I will tread carefully. Related, I've found a problem w/ my above pull req., which is that the timeout event will be fired on an idle connection timeout, as well as a timeout trying to connect. Only the latter should trigger a connection issue. I found there's a related pull req here #93 which I'll take a look at. Looks like it may only need tests. |
Attempts to fix #16
From what I could tell, the case when it's not possible to connect to any memcached servers is not handled, and this type of issue is not logged. This pull request adds a handler for the error event on the stream, which calls connectionIssue. Additionally, if no healthy servers are available, and error will be returned. If the client implementation comes across this error, it can be handled.
@3rd-Eden is this on the right track?