libmemcached

Merge lp:~jc.redoutey/libmemcached/failure_number_fix into lp:~tangent-org/libmemcached/trunk

  1. failure_number_fix
  2. Merge into trunk
Proposed by JC Redoutey
Status: Work in progress
Proposed branch: lp:~jc.redoutey/libmemcached/failure_number_fix
Merge into: lp:~tangent-org/libmemcached/trunk
Diff against target: 131 lines
2 files modified
libmemcached/memcached_quit.c (+8/-8)
tests/function.c (+54/-0)
To merge this branch: bzr merge lp:~jc.redoutey/libmemcached/failure_number_fix
Reviewer Review Type Date Requested Status
Brian Aker Needs Fixing
Review via email: mp+13167@code.launchpad.net
To post a comment you must log in.
Revision history for this message
JC Redoutey (jc.redoutey) wrote :

This small patch aims at preventing increasing the retry counter when quitting a memcached server is intentional.

Reply
Revision history for this message
Brian Aker (brianaker) :
review: Approve
Reply
Revision history for this message
Brian Aker (brianaker) wrote :

This is causing random test failures once applied to my local Fedora box. Don't you want to clone the memc in the test?

review: Needs Fixing
Reply
lp:~jc.redoutey/libmemcached/failure_number_fix updated
601. By Jean-Charles Redoutey <jc@Jayce2009lnx>

now cloning memcached_st

Revision history for this message
Brian Aker (brianaker) wrote :

So how does this test compare to what we already have? (Which looks to be incomplete, so I suspect this is better).

BTW I will work out an example in the tests which allow you to have memcached_st release all but one server.

Reply
Revision history for this message
Brian Aker (brianaker) wrote :

I've extracted this test case (it passes and does show different behavior from the current version). Thanks.

Reply

Unmerged revisions

601. By Jean-Charles Redoutey <jc@Jayce2009lnx>

now cloning memcached_st

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'libmemcached/memcached_quit.c'
2--- libmemcached/memcached_quit.c 2009-07-18 17:37:40 +0000
3+++ libmemcached/memcached_quit.c 2009-10-12 19:59:10 +0000
4@@ -2,10 +2,10 @@
5
6 /*
7 This closes all connections (forces flush of input as well).
8-
9- Maybe add a host specific, or key specific version?
10-
11- The reason we send "quit" is that in case we have buffered IO, this
12+
13+ Maybe add a host specific, or key specific version?
14+
15+ The reason we send "quit" is that in case we have buffered IO, this
16 will force data to be completed.
17 */
18
19@@ -18,7 +18,7 @@
20 memcached_return rc;
21 char buffer[MEMCACHED_MAX_BUFFER];
22
23- if (ptr->root->flags & MEM_BINARY_PROTOCOL)
24+ if (ptr->root->flags & MEM_BINARY_PROTOCOL)
25 {
26 protocol_binary_request_quit request = {.bytes= {0}};
27 request.message.header.request.magic = PROTOCOL_BINARY_REQ;
28@@ -30,7 +30,7 @@
29 rc= memcached_do(ptr, "quit\r\n", 6, 1);
30
31 WATCHPOINT_ASSERT(rc == MEMCACHED_SUCCESS || rc == MEMCACHED_FETCH_NOTFINISHED);
32-
33+
34 /* read until socket is closed, or there is an error
35 * closing the socket before all data is read
36 * results in server throwing away all data which is
37@@ -49,14 +49,14 @@
38 memcached_server_response_reset(ptr);
39 }
40
41- ptr->server_failure_counter++;
42+ if(io_death) ptr->server_failure_counter++;
43 }
44
45 void memcached_quit(memcached_st *ptr)
46 {
47 unsigned int x;
48
49- if (ptr->hosts == NULL ||
50+ if (ptr->hosts == NULL ||
51 ptr->number_of_hosts == 0)
52 return;
53
54
55=== modified file 'tests/function.c'
56--- tests/function.c 2009-10-12 16:10:17 +0000
57+++ tests/function.c 2009-10-12 19:59:10 +0000
58@@ -4732,6 +4732,7 @@
59 return TEST_SUCCESS;
60 }
61
62+<<<<<<< TREE
63
64
65 /* Test memcached_server_get_last_disconnect
66@@ -4786,6 +4787,58 @@
67 }
68
69
70+=======
71+
72+/*
73+ * This tests ensures expected disconnections (for some behavior changes
74+ * for instance) do not wrongly increase failure counter
75+ */
76+static test_return_t wrong_failure_counter_test(memcached_st *memc)
77+{
78+ memcached_return rc;
79+
80+ memcached_st *memc_clone;
81+ memc_clone= memcached_clone(NULL, memc);
82+ assert(memc_clone);
83+
84+ /* Set value to force connection to the server */
85+ const char *key= "marmotte";
86+ const char *value= "milka";
87+ char *string = NULL;
88+ size_t string_length;
89+ uint32_t flags;
90+
91+ rc= memcached_set(memc_clone, key, strlen(key),
92+ value, strlen(value),
93+ (time_t)0, (uint32_t)0);
94+ assert(rc == MEMCACHED_SUCCESS || rc == MEMCACHED_BUFFERED);
95+
96+
97+ /* put failure limit to 1 */
98+ rc= memcached_behavior_set(memc_clone, MEMCACHED_BEHAVIOR_SERVER_FAILURE_LIMIT, 1);
99+ assert(rc == MEMCACHED_SUCCESS);
100+ /* Put a retry timeout to effectively activate failure_limit effect */
101+ rc= memcached_behavior_set(memc_clone, MEMCACHED_BEHAVIOR_RETRY_TIMEOUT, 1);
102+ assert(rc == MEMCACHED_SUCCESS);
103+ /* change behavior that triggers memcached_quit()*/
104+ rc= memcached_behavior_set(memc_clone, MEMCACHED_BEHAVIOR_TCP_NODELAY, 1);
105+ assert(rc == MEMCACHED_SUCCESS);
106+
107+
108+ /* Check if we still are connected */
109+ string= memcached_get(memc_clone, key, strlen(key),
110+ &string_length, &flags, &rc);
111+
112+ assert(rc == MEMCACHED_SUCCESS);
113+ assert(string);
114+ free(string);
115+
116+ return TEST_SUCCESS;
117+}
118+
119+
120+
121+>>>>>>> MERGE-SOURCE
122 test_st udp_setup_server_tests[] ={
123 {"set_udp_behavior_test", 0, set_udp_behavior_test},
124 {"add_tcp_server_udp_client_test", 0, add_tcp_server_udp_client_test},
125@@ -4930,6 +4983,7 @@
126 {"user_supplied_bug19", 1, user_supplied_bug19 },
127 {"user_supplied_bug20", 1, user_supplied_bug20 },
128 {"user_supplied_bug21", 1, user_supplied_bug21 },
129+ {"wrong_failure_counter_test", 1, wrong_failure_counter_test},
130 {0, 0, 0}
131 };
132

Subscribers

People subscribed via source and target branches

to all changes: