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

Improve the support of unix socket #642

Merged
merged 3 commits into from Aug 9, 2014
Merged

Conversation

jackhftang
Copy link
Contributor

I noted the issue #204. But, I got an error using redis.createClient('/tmp/redis.sock'). Not sure if this is because the interface of net.createConnection() changed.

Anyway, IMHO, unix socket is a useful feature that need to be there. It provides better isolation, faster performance. And yet this is not that much of work.

I removed client.host and client.port, because they do not fit for unix socket.
And add client.address and client.connectionOption. The former is better to reference addres and the later to reference the way to create connection.

I also added new test case UNIX_SOCKET and updated the testcase CHECK_LIST. This pull request pass all test cases.

Finally, README is updated.

delete client.host
delete client.port
add client.address
add client.connectionOptions

add tests.UNIX_SOCKET

update all error message to use client.address
update retry connection
@jackhftang
Copy link
Contributor Author

By the way, it seems to overrlap/fix with issues
issue #632
pull #638
pull #639
pull #625

@brycebaril
Copy link
Contributor

This is awesome! Very complete w/ tests and docs. Releasing as 0.12.0 imminently.

@@ -610,11 +635,16 @@ tests.CLIENT_LIST = function() {
return next(name);
}

var pattern = /^add=/;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, typo here that breaks tests on older versions of Redis. I'll take care of this before I release.

@brycebaril brycebaril merged commit 5e1cebe into redis:master Aug 9, 2014
@brycebaril
Copy link
Contributor

Thanks! Published as 0.12.0

@jackhftang
Copy link
Contributor Author

This is my careless. I just saw a lot createClient(null,null, {...}) in test cases. I didn't understand the purpose of this and think this is ugly, so I didn't write in README. Actually, in order to be backward compatible, I did handle createClient(null,null,{...}) , but didn't know I need to include createClient(undefined, undefined,{...}) also.

Solution 1

Adding typeof arg0 === 'undefined' && typeof arg1 === 'undefined' in last case can handle development situation, but one must provide both PORT and HOST. This is not fully backward-compatible.

Solution 2

Don't throw Error('unknown type of connection in createClient()') in else case, default to create(arg0 || default_port, arg1 || default_host, arg2). This should be fully backward-compatible.

Solution 3

separate and expose createClientUnix and createClientTcp. Don't unify the creation of unix and tcp socket. No ambiguous.

Solution 4

Remain unchanged. Then one need to write something like this.

var port = process.env.PORT || 6379;
var host = process.env.HOST || '127.0.0.1';
var client = createClient(port,host)

Personally, i prefer solution 1. As solution 2 adding support to something like createClient('3000',undefined) and create(undefined, 'localhost') are too vague.

Let me know which do you prefer. I am happy to submit pull request. =]

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.

None yet

3 participants