Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

error on timeout now handled by jackpot #136

Merged
merged 2 commits into from
Aug 2, 2013

Conversation

ianshward
Copy link
Collaborator

Fixes broken tests in #135 ... now that Jackpot returns error on timeout when trying to connect socket, node-memcached no longer needs to do so.

@ianshward
Copy link
Collaborator Author

Hmm, I don't think this is quite right actually, I'll follow up...

@ianshward
Copy link
Collaborator Author

Working on some changes with this. It seems overall that Jackpot should, and does, handle any pre 'connect' errors, and treats a timeout while trying to connect as an error, and that node-memcached should handle any post 'connect' errors by reporting them to connectionIssue().

@ianshward
Copy link
Collaborator Author

I updated the pull. The idea is that Jackpot handled pre-connect timeouts and errors, while, node-memcached needs to listen for any errors on the socket post-connect.

@3rd-Eden
Copy link
Owner

3rd-Eden commented Aug 2, 2013

Looks really slick, thanks for the work on this. Do you think it makes sense to move the removal/releasing of the connections in to Jackpot instead of adding logic against it in memcached?

3rd-Eden added a commit that referenced this pull request Aug 2, 2013
error on timeout now handled by jackpot
@3rd-Eden 3rd-Eden merged commit 5a3b924 into 3rd-Eden:master Aug 2, 2013
@ianshward
Copy link
Collaborator Author

@3rd-Eden thanks. Yes, I think it could make sense. Something related I've been thinking about is the ability to differentiate which errors should be considered "fatal" errors, and which errors should just be considered warnings, whereby only "fatal" errors would put nodes into a failed state. For example, if you try to SET an item larger than the allowed size, node-memcached emits an error, which may not be considered "fatal" / worth marking a node failed. Have you thought about working this into the issue logging system? Will issue logging live in failover in 1.0?

@ianshward ianshward deleted the fix-timeout-test branch August 5, 2013 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants