Merge lp:~mattknox/libmemcached/twitter-compat into lp:~tangent-org/libmemcached/trunk

Proposed by matt knox on 2009-06-29
Status: Merged
Merged at revision: not available
Proposed branch: lp:~mattknox/libmemcached/twitter-compat
Merge into: lp:~tangent-org/libmemcached/trunk
Diff against target: None lines
To merge this branch: bzr merge lp:~mattknox/libmemcached/twitter-compat
Reviewer Review Type Date Requested Status
Libmemcached-developers 2009-06-29 Pending
Review via email: mp+8030@code.launchpad.net
To post a comment you must log in.
matt knox (mattknox) wrote :

The code in this branch caches memcached server errors in the memcached_server_st struct to give better visibility into failures. I've tested it both with its own suite and that of the ruby interface to it, and also run it in a development environment. So far it looks fine on all counts.

543. By matt knox on 2009-06-30

oops! Now I'm using the malloc api, rather than direct malloc

544. By matt knox on 2009-06-30

removed free

Brian Aker (brianaker) wrote :

> The code in this branch caches memcached server errors in the
> memcached_server_st struct to give better visibility into failures. I've
> tested it both with its own suite and that of the ruby interface to it, and
> also run it in a development environment. So far it looks fine on all counts.

There are a few problems with this.

1) You don't test for memory failures.
2) Lets say you have a failure, how do you know which server it was that had the error?
3) How do you clean up the error (aka reset the error)?
4) There is no function to encapsulate the error message. Which means any future changes to the structure are going to break if any renaming occurs.

I'll clean this up a bit... but something needs to be figured out so that you know via some vector about which servers got errors in the last run.

I can certainly see the usefulness in this for debugging on production sites.

545. By matt knox on 2009-07-13

corrected a spelling error in libmemcached_examples

matt knox (mattknox) wrote :

hey-I just made what I think are some improvements to the twitter-compat
branch. Among other things, I pulled the setting of the cached_server_error
out into its own function. I'll wrap the access also.

On Wed, Jul 15, 2009 at 2:38 AM, Brian Aker <email address hidden> wrote:

> > The code in this branch caches memcached server errors in the
> > memcached_server_st struct to give better visibility into failures. I've
> > tested it both with its own suite and that of the ruby interface to it,
> and
> > also run it in a development environment. So far it looks fine on all
> counts.
>
> There are a few problems with this.
>
> 1) You don't test for memory failures.
> 2) Lets say you have a failure, how do you know which server it was that
> had the error?
> 3) How do you clean up the error (aka reset the error)?
> 4) There is no function to encapsulate the error message. Which means any
> future changes to the structure are going to break if any renaming occurs.
>
> I'll clean this up a bit... but something needs to be figured out so that
> you know via some vector about which servers got errors in the last run.
>
> I can certainly see the usefulness in this for debugging on production
> sites.
>
>
> --
>
> https://code.launchpad.net/~mattknox/libmemcached/twitter-compat/+merge/8030<https://code.launchpad.net/%7Emattknox/libmemcached/twitter-compat/+merge/8030>
> You are the owner of lp:~mattknox/libmemcached/twitter-compat.
>

--
(def (eval e l d c)
 (if (atom? e)
     ((ahandler (type e)) e l d c)
     (eval (car e) l d
             (fun (x)
                (evapp x (cdr e) l d c)))))

matt knox (mattknox) wrote :

I've made several changes, including wrapping the cached_server_error in a
function and moving all that out into its own set of files, with docs and
such. How's that look now?

matt

On Wed, Jul 15, 2009 at 2:38 AM, Brian Aker <email address hidden> wrote:

> > The code in this branch caches memcached server errors in the
> > memcached_server_st struct to give better visibility into failures. I've
> > tested it both with its own suite and that of the ruby interface to it,
> and
> > also run it in a development environment. So far it looks fine on all
> counts.
>
> There are a few problems with this.
>
> 1) You don't test for memory failures.
> 2) Lets say you have a failure, how do you know which server it was that
> had the error?
> 3) How do you clean up the error (aka reset the error)?
> 4) There is no function to encapsulate the error message. Which means any
> future changes to the structure are going to break if any renaming occurs.
>
> I'll clean this up a bit... but something needs to be figured out so that
> you know via some vector about which servers got errors in the last run.
>
> I can certainly see the usefulness in this for debugging on production
> sites.
>
>
> --
>
> https://code.launchpad.net/~mattknox/libmemcached/twitter-compat/+merge/8030<https://code.launchpad.net/%7Emattknox/libmemcached/twitter-compat/+merge/8030>
> You are the owner of lp:~mattknox/libmemcached/twitter-compat.
>

--
(def (eval e l d c)
 (if (atom? e)
     ((ahandler (type e)) e l d c)
     (eval (car e) l d
             (fun (x)
                (evapp x (cdr e) l d c)))))

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libmemcached/memcached_response.c'
2--- libmemcached/memcached_response.c 2009-06-15 00:51:32 +0000
3+++ libmemcached/memcached_response.c 2009-06-29 20:51:23 +0000
4@@ -238,7 +238,16 @@
5 return MEMCACHED_STAT;
6 }
7 else if (buffer[1] == 'E')
8- return MEMCACHED_SERVER_ERROR;
9+ {
10+ /* SERVER_ERROR */
11+ char *startptr= buffer + 13, *endptr= startptr;
12+ while (*endptr != '\r' && *endptr != '\n') endptr++;
13+ if (ptr->cached_server_error) free(ptr->cached_server_error);
14+ ptr->cached_server_error= malloc(endptr - startptr + 1);
15+ memcpy(ptr->cached_server_error, startptr, endptr - startptr);
16+ ptr->cached_server_error[endptr - startptr]= 0;
17+ return MEMCACHED_SERVER_ERROR;
18+ }
19 else if (buffer[1] == 'T')
20 return MEMCACHED_STORED;
21 else
22
23=== modified file 'libmemcached/memcached_server.c'
24--- libmemcached/memcached_server.c 2009-06-10 10:29:06 +0000
25+++ libmemcached/memcached_server.c 2009-06-29 20:51:23 +0000
26@@ -53,6 +53,12 @@
27 {
28 memcached_quit_server(ptr, 0);
29
30+ if (ptr->cached_server_error)
31+ {
32+ free(ptr->cached_server_error);
33+ ptr->cached_server_error= NULL;
34+ }
35+
36 if (ptr->address_info)
37 {
38 freeaddrinfo(ptr->address_info);
39@@ -70,14 +76,24 @@
40 */
41 memcached_server_st *memcached_server_clone(memcached_server_st *clone, memcached_server_st *ptr)
42 {
43+ memcached_server_st *rv = NULL;
44+
45 /* We just do a normal create if ptr is missing */
46 if (ptr == NULL)
47 return NULL;
48
49- /* TODO We should check return type */
50- return memcached_server_create_with(ptr->root, clone,
51- ptr->hostname, ptr->port, ptr->weight,
52- ptr->type);
53+ rv = memcached_server_create_with(ptr->root, clone,
54+ ptr->hostname, ptr->port, ptr->weight,
55+ ptr->type);
56+ if (rv != NULL)
57+ {
58+ rv->cached_errno= ptr->cached_errno;
59+ if (ptr->cached_server_error)
60+ rv->cached_server_error= strdup(ptr->cached_server_error);
61+ }
62+
63+ return rv;
64+
65 }
66
67 memcached_return memcached_server_cursor(memcached_st *ptr,
68
69=== modified file 'libmemcached/memcached_server.h'
70--- libmemcached/memcached_server.h 2009-05-20 15:58:02 +0000
71+++ libmemcached/memcached_server.h 2009-06-29 20:51:23 +0000
72@@ -21,6 +21,7 @@
73 unsigned int port;
74 int cached_errno;
75 int fd;
76+ char *cached_server_error;
77 uint32_t io_bytes_sent; /* # bytes sent since last read */
78 uint32_t server_failure_counter;
79 uint32_t weight;

Subscribers

People subscribed via source and target branches

to all changes: