Redis SELECT on connect #294

Merged
merged 5 commits into from Dec 22, 2014

Conversation

Projects
None yet
3 participants
@arnecls

arnecls commented Dec 11, 2014

This is an improvement upon pull request #217. Some of the source here is based on this pull request.
We have tested this working on FreeBSD 10 (live traffic), Mac OS X (local dev) and Linux (virtual machine).

Improvements over #217:

  • The select command is pushed to the standard server message queue
  • This removes the need to check for the response (already done by existing code)
  • The time and ownership of the response is now clear, too
  • Works on OS X and FreeBSD 10 (which blocked connections with the original patch)

You can add an option "redis_db" to the configuration file to use this patch.
All servers in a pool will select the same database so consistent hashing is possible.
The select command is still not allowed during "normal" runtime as this would lead to confusion.

To compile on FreeBSD10 pull request #293 is required.

@arnecls arnecls changed the title from Redis select db to Redis SELECT on connect Dec 11, 2014

- Fixed select message type
- Added log notice for triggering a select
- Fixed a missing prototype warning
src/nc_connection.c
@@ -243,6 +244,7 @@ conn_get(void *owner, bool client, bool redis)
conn->dequeue_inq = req_server_dequeue_imsgq;
conn->enqueue_outq = req_server_enqueue_omsgq;
conn->dequeue_outq = req_server_dequeue_omsgq;
+ conn->initialize = server_conn_init;

This comment has been minimized.

@manjuraj

manjuraj Dec 17, 2014

Collaborator

can we call this init?

@manjuraj

manjuraj Dec 17, 2014

Collaborator

can we call this init?

src/nc_message.h
@@ -162,6 +162,7 @@ typedef enum msg_parse_result {
ACTION( REQ_REDIS_PING ) /* redis requests - ping/quit */ \
ACTION( REQ_REDIS_QUIT) \
ACTION( REQ_REDIS_AUTH) \
+ ACTION( REQ_REDIS_SELECT) /* Only during init */ \

This comment has been minimized.

@manjuraj

manjuraj Dec 17, 2014

Collaborator

s/Only/only/

@manjuraj

manjuraj Dec 17, 2014

Collaborator

s/Only/only/

src/nc_request.c
@@ -674,6 +698,12 @@ req_send_next(struct context *ctx, struct conn *conn)
server_connected(ctx, conn);
}
+ // if the connection is dead for some reason (e.g. it hasn't been initialized

This comment has been minimized.

@manjuraj

manjuraj Dec 17, 2014

Collaborator

we only use comments of form /* */ in twemproxy

@manjuraj

manjuraj Dec 17, 2014

Collaborator

we only use comments of form /* */ in twemproxy

src/nc_server.c
@@ -168,6 +169,63 @@ server_deinit(struct array *server)
array_deinit(server);
}
+void
+server_conn_init(struct context *ctx, struct conn *conn, struct server *server)

This comment has been minimized.

@manjuraj

manjuraj Dec 17, 2014

Collaborator

since this only affects redis, can we have two init handlers for memcache (memcache_conn_init) and redis (redis_conn_init) and move the code from server_conn_init to redis_conn_init

@manjuraj

manjuraj Dec 17, 2014

Collaborator

since this only affects redis, can we have two init handlers for memcache (memcache_conn_init) and redis (redis_conn_init) and move the code from server_conn_init to redis_conn_init

src/nc_server.c
+ ? (int)log10(server->owner->redis_db) + 1
+ : 1;
+
+ commandlen = snprintf(command, sizeof(command), "*2\r\n$6\r\nSELECT\r\n$%d\r\n%d\r\n", digits, server->owner->redis_db);

This comment has been minimized.

@manjuraj

manjuraj Dec 17, 2014

Collaborator

can you use nc_snprintf

@manjuraj

manjuraj Dec 17, 2014

Collaborator

can you use nc_snprintf

src/nc_server.c
+
+ // Copy message to buffer
+
+ strcpy((char*)mbuf->last, command);

This comment has been minimized.

@manjuraj

manjuraj Dec 17, 2014

Collaborator

use mbuf_* methods to do the copy

@manjuraj

manjuraj Dec 17, 2014

Collaborator

use mbuf_* methods to do the copy

src/nc_server.c
+ mbuf->last += commandlen;
+ msg->mlen += (uint32_t)commandlen;
+
+ // Set parser states and swallow to true (no client to respond)

This comment has been minimized.

@manjuraj

manjuraj Dec 17, 2014

Collaborator

use /* */ comment

@manjuraj

manjuraj Dec 17, 2014

Collaborator

use /* */ comment

@@ -1221,6 +1226,10 @@ conf_validate_pool(struct conf *cf, struct conf_pool *cp)
cp->redis = CONF_DEFAULT_REDIS;
}
+ if (cp->redis_db == CONF_UNSET_NUM) {
+ cp->redis_db = CONF_DEFAULT_REDIS_DB;

This comment has been minimized.

@manjuraj

manjuraj Dec 17, 2014

Collaborator

should we put a cap on maximum number of redis db -- like redis_db must be less than 64

@manjuraj

manjuraj Dec 17, 2014

Collaborator

should we put a cap on maximum number of redis db -- like redis_db must be less than 64

This comment has been minimized.

@arnecls

arnecls Dec 17, 2014

As far as I know there is no cap on the db setting in redis

@arnecls

arnecls Dec 17, 2014

As far as I know there is no cap on the db setting in redis

src/nc_server.c
+ struct msg *msg;
+ struct mbuf *mbuf;
+
+ digits = (server->owner->redis_db >= 10)

This comment has been minimized.

@manjuraj

manjuraj Dec 17, 2014

Collaborator

can we compute number of digits in nc_conf.c and store it in conf structure -- no need to recompute it on every connection request

@manjuraj

manjuraj Dec 17, 2014

Collaborator

can we compute number of digits in nc_conf.c and store it in conf structure -- no need to recompute it on every connection request

This comment has been minimized.

@arnecls

arnecls Dec 17, 2014

Of course we could, but as this command is not heavily frequented it should not be much of a performance issue. So I guess adding a field to the config is more wasteful than doing the calculation in place, especially as the value is not used elsewhere.
Sadly the redis protocol integer notation does not work here : /

@arnecls

arnecls Dec 17, 2014

Of course we could, but as this command is not heavily frequented it should not be much of a performance issue. So I guess adding a field to the config is more wasteful than doing the calculation in place, especially as the value is not used elsewhere.
Sadly the redis protocol integer notation does not work here : /

src/nc_server.c
+
+ if (conn->redis && server->owner->redis_db > 0)
+ {
+ char command[64];

This comment has been minimized.

@manjuraj

manjuraj Dec 17, 2014

Collaborator

use uint8_t

@manjuraj

manjuraj Dec 17, 2014

Collaborator

use uint8_t

arnecls added some commits Dec 17, 2014

Adjusted code based on recommedations:
- server_conn_init moved to redis plugin (redis_conn_init)
- renamed conn->initialize to conn->init
- fixed typos and comments
- removed a now obsolete check from #217 in nc_request.c
Warning on failed select
- added a callback that can inspect a req/rsp before it is swallowed
- this callback is now used with redis so it can check on failed selects

manjuraj added a commit that referenced this pull request Dec 22, 2014

@manjuraj manjuraj merged commit 37ca913 into twitter:master Dec 22, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@idning

This comment has been minimized.

Show comment
Hide comment
@idning

idning Dec 24, 2014

Contributor

what we did here is:

allow to select a (default) db on redis,

we still can not support a common ’SELECT’ command, I think this feature make user confused.

Contributor

idning commented Dec 24, 2014

what we did here is:

allow to select a (default) db on redis,

we still can not support a common ’SELECT’ command, I think this feature make user confused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment