From 0ec6fc3c490da878f1ad597c9501f5db686192a3 Mon Sep 17 00:00:00 2001 From: Brian Aker Date: Sat, 28 Jan 2012 14:13:16 -0800 Subject: [PATCH] Fix it so that we use the result object for storing the numeric result that we eventually returned. --- libmemcached-1.0/struct/result.h | 1 + libmemcached/auto.cc | 88 ++++++++++++------------ libmemcached/response.cc | 37 +++++----- libmemcached/response.h | 4 +- libmemcached/result.cc | 3 + tests/libmemcached-1.0/mem_functions.cc | 1 + tests/libmemcached-1.0/replication.cc | 89 +++++++++++++++++++++++++ tests/replication.h | 14 +--- 8 files changed, 157 insertions(+), 80 deletions(-) diff --git a/libmemcached-1.0/struct/result.h b/libmemcached-1.0/struct/result.h index 2b2f4996..c3bfddb5 100644 --- a/libmemcached-1.0/struct/result.h +++ b/libmemcached-1.0/struct/result.h @@ -44,6 +44,7 @@ struct memcached_result_st { uint64_t item_cas; struct memcached_st *root; memcached_string_st value; + uint64_t numeric_value; uint64_t count; char item_key[MEMCACHED_MAX_KEY]; struct { diff --git a/libmemcached/auto.cc b/libmemcached/auto.cc index c64ccda4..964e8971 100644 --- a/libmemcached/auto.cc +++ b/libmemcached/auto.cc @@ -37,12 +37,36 @@ #include +static void auto_response(memcached_server_write_instance_st instance, const bool reply, memcached_return_t& rc, uint64_t* value) +{ + // If the message was successfully sent, then get the response, otherwise + // fail. + if (memcached_success(rc)) + { + if (reply == false) + { + *value= UINT64_MAX; + return; + } + + rc= memcached_response(instance, &instance->root->result); + } + + if (memcached_success(rc)) + { + *value= instance->root->result.numeric_value; + } + else + { + *value= UINT64_MAX; + } +} + static memcached_return_t text_incr_decr(memcached_server_write_instance_st instance, const bool is_incr, const char *key, size_t key_length, const uint64_t offset, - const bool reply, - uint64_t& numeric_value) + const bool reply) { char buffer[MEMCACHED_DEFAULT_COMMAND_SIZE]; @@ -69,22 +93,7 @@ static memcached_return_t text_incr_decr(memcached_server_write_instance_st inst vector[1].buffer= "decr "; } - memcached_return_t rc= memcached_vdo(instance, vector, 7, true); - - if (reply == false) - { - return MEMCACHED_SUCCESS; - } - - if (memcached_failed(rc)) - { - numeric_value= UINT64_MAX; - return rc; - } - - rc= memcached_response(instance, buffer, sizeof(buffer), NULL, numeric_value); - - return memcached_set_error(*instance, rc, MEMCACHED_AT); + return memcached_vdo(instance, vector, 7, true); } static memcached_return_t binary_incr_decr(memcached_server_write_instance_st instance, @@ -93,8 +102,7 @@ static memcached_return_t binary_incr_decr(memcached_server_write_instance_st in const uint64_t offset, const uint64_t initial, const uint32_t expiration, - const bool reply, - uint64_t *value) + const bool reply) { if (reply == false) { @@ -128,19 +136,7 @@ static memcached_return_t binary_incr_decr(memcached_server_write_instance_st in { key, key_length } }; - memcached_return_t rc; - if (memcached_failed(rc= memcached_vdo(instance, vector, 4, true))) - { - memcached_io_reset(instance); - return MEMCACHED_WRITE_FAILURE; - } - - if (reply == false) - { - return MEMCACHED_SUCCESS; - } - - return memcached_response(instance, (char*)value, sizeof(*value), NULL); + return memcached_vdo(instance, vector, 4, true); } memcached_return_t memcached_increment(memcached_st *ptr, @@ -193,14 +189,15 @@ memcached_return_t memcached_increment_by_key(memcached_st *ptr, rc= binary_incr_decr(instance, PROTOCOL_BINARY_CMD_INCREMENT, key, key_length, uint64_t(offset), 0, MEMCACHED_EXPIRATION_NOT_ADD, - reply, - value); + reply); } else { - rc= text_incr_decr(instance, true, key, key_length, offset, reply, *value); + rc= text_incr_decr(instance, true, key, key_length, offset, reply); } + auto_response(instance, reply, rc, value); + LIBMEMCACHED_MEMCACHED_INCREMENT_END(); return rc; @@ -241,14 +238,15 @@ memcached_return_t memcached_decrement_by_key(memcached_st *ptr, rc= binary_incr_decr(instance, PROTOCOL_BINARY_CMD_DECREMENT, key, key_length, offset, 0, MEMCACHED_EXPIRATION_NOT_ADD, - reply, - value); + reply); } else { - rc= text_incr_decr(instance, false, key, key_length, offset, reply, *value); + rc= text_incr_decr(instance, false, key, key_length, offset, reply); } + auto_response(instance, reply, rc, value); + LIBMEMCACHED_MEMCACHED_DECREMENT_END(); return rc; @@ -305,8 +303,8 @@ memcached_return_t memcached_increment_with_initial_by_key(memcached_st *ptr, rc= binary_incr_decr(instance, PROTOCOL_BINARY_CMD_INCREMENT, key, key_length, offset, initial, uint32_t(expiration), - reply, - value); + reply); + } else { @@ -314,6 +312,8 @@ memcached_return_t memcached_increment_with_initial_by_key(memcached_st *ptr, memcached_literal_param("memcached_increment_with_initial_by_key() is not supported via the ASCII protocol")); } + auto_response(instance, reply, rc, value); + LIBMEMCACHED_MEMCACHED_INCREMENT_WITH_INITIAL_END(); return rc; @@ -371,8 +371,8 @@ memcached_return_t memcached_decrement_with_initial_by_key(memcached_st *ptr, rc= binary_incr_decr(instance, PROTOCOL_BINARY_CMD_DECREMENT, key, key_length, offset, initial, uint32_t(expiration), - reply, - value); + reply); + } else { @@ -380,6 +380,8 @@ memcached_return_t memcached_decrement_with_initial_by_key(memcached_st *ptr, memcached_literal_param("memcached_decrement_with_initial_by_key() is not supported via the ASCII protocol")); } + auto_response(instance, reply, rc, value); + LIBMEMCACHED_MEMCACHED_INCREMENT_WITH_INITIAL_END(); return rc; diff --git a/libmemcached/response.cc b/libmemcached/response.cc index ac1fb057..437c4cc9 100644 --- a/libmemcached/response.cc +++ b/libmemcached/response.cc @@ -182,10 +182,8 @@ read_error: static memcached_return_t textual_read_one_response(memcached_server_write_instance_st ptr, char *buffer, const size_t buffer_length, - memcached_result_st *result, - uint64_t& numeric_value) + memcached_result_st *result) { - numeric_value= UINT64_MAX; size_t total_read; memcached_return_t rc= memcached_io_readline(ptr, buffer, buffer_length, total_read); @@ -381,16 +379,18 @@ static memcached_return_t textual_read_one_response(memcached_server_write_insta if (auto_return_value == ULLONG_MAX and errno == ERANGE) { + result->numeric_value= UINT64_MAX; return memcached_set_error(*ptr, MEMCACHED_UNKNOWN_READ_FAILURE, MEMCACHED_AT, memcached_literal_param("Numeric response was out of range")); } else if (errno == EINVAL) { + result->numeric_value= UINT64_MAX; return memcached_set_error(*ptr, MEMCACHED_UNKNOWN_READ_FAILURE, MEMCACHED_AT, memcached_literal_param("Numeric response was out of range")); } - numeric_value= uint64_t(auto_return_value); + result->numeric_value= uint64_t(auto_return_value); WATCHPOINT_STRING(buffer); return MEMCACHED_SUCCESS; @@ -509,21 +509,20 @@ static memcached_return_t binary_read_one_response(memcached_server_write_instan case PROTOCOL_BINARY_CMD_INCREMENT: case PROTOCOL_BINARY_CMD_DECREMENT: { - if (bodylen != sizeof(uint64_t) or buffer_length != sizeof(uint64_t)) + if (bodylen != sizeof(uint64_t)) { + result->numeric_value= UINT64_MAX; return memcached_set_error(*ptr, MEMCACHED_UNKNOWN_READ_FAILURE, MEMCACHED_AT); } - WATCHPOINT_ASSERT(bodylen == buffer_length); uint64_t val; if ((rc= memcached_safe_read(ptr, &val, sizeof(val))) != MEMCACHED_SUCCESS) { - WATCHPOINT_ERROR(rc); + result->numeric_value= UINT64_MAX; return MEMCACHED_UNKNOWN_READ_FAILURE; } - val= memcached_ntohll(val); - memcpy(buffer, &val, sizeof(val)); + result->numeric_value= memcached_ntohll(val); } break; @@ -694,8 +693,7 @@ static memcached_return_t binary_read_one_response(memcached_server_write_instan static memcached_return_t _read_one_response(memcached_server_write_instance_st ptr, char *buffer, const size_t buffer_length, - memcached_result_st *result, - uint64_t& numeric_value) + memcached_result_st *result) { memcached_server_response_decrement(ptr); @@ -712,7 +710,7 @@ static memcached_return_t _read_one_response(memcached_server_write_instance_st } else { - rc= textual_read_one_response(ptr, buffer, buffer_length, result, numeric_value); + rc= textual_read_one_response(ptr, buffer, buffer_length, result); assert(rc != MEMCACHED_PROTOCOL_ERROR); } @@ -727,7 +725,6 @@ static memcached_return_t _read_one_response(memcached_server_write_instance_st memcached_return_t memcached_read_one_response(memcached_server_write_instance_st ptr, memcached_result_st *result) { - uint64_t numeric_value; char buffer[SMALL_STRING_LEN]; if (memcached_is_udp(ptr->root)) @@ -736,22 +733,20 @@ memcached_return_t memcached_read_one_response(memcached_server_write_instance_s } - return _read_one_response(ptr, buffer, sizeof(buffer), result, numeric_value); + return _read_one_response(ptr, buffer, sizeof(buffer), result); } memcached_return_t memcached_response(memcached_server_write_instance_st ptr, - char *buffer, size_t buffer_length, memcached_result_st *result) { - uint64_t numeric_value; + char buffer[1024]; - return memcached_response(ptr, buffer, buffer_length, result, numeric_value); + return memcached_response(ptr, buffer, sizeof(buffer), result); } memcached_return_t memcached_response(memcached_server_write_instance_st ptr, char *buffer, size_t buffer_length, - memcached_result_st *result, - uint64_t& numeric_value) + memcached_result_st *result) { if (memcached_is_udp(ptr->root)) { @@ -778,7 +773,7 @@ memcached_return_t memcached_response(memcached_server_write_instance_st ptr, while (memcached_server_response_count(ptr) > 1) { - memcached_return_t rc= _read_one_response(ptr, buffer, buffer_length, junked_result_ptr, numeric_value); + memcached_return_t rc= _read_one_response(ptr, buffer, buffer_length, junked_result_ptr); // @TODO should we return an error on another but a bad read case? if ( @@ -805,5 +800,5 @@ memcached_return_t memcached_response(memcached_server_write_instance_st ptr, memcached_result_free(junked_result_ptr); } - return _read_one_response(ptr, buffer, buffer_length, result, numeric_value); + return _read_one_response(ptr, buffer, buffer_length, result); } diff --git a/libmemcached/response.h b/libmemcached/response.h index d9abdb8a..8827aea0 100644 --- a/libmemcached/response.h +++ b/libmemcached/response.h @@ -42,10 +42,8 @@ memcached_return_t memcached_read_one_response(memcached_server_write_instance_s memcached_result_st *result); memcached_return_t memcached_response(memcached_server_write_instance_st ptr, - char *buffer, size_t buffer_length, memcached_result_st *result); memcached_return_t memcached_response(memcached_server_write_instance_st ptr, char *buffer, size_t buffer_length, - memcached_result_st *result, - uint64_t& numeric_value); + memcached_result_st *result); diff --git a/libmemcached/result.cc b/libmemcached/result.cc index a3438c08..444a75f4 100644 --- a/libmemcached/result.cc +++ b/libmemcached/result.cc @@ -52,6 +52,7 @@ static inline void _result_init(memcached_result_st *self, self->key_length= 0; self->item_cas= 0; self->root= memc; + self->numeric_value= UINT64_MAX; self->count= 0; self->item_key[0]= 0; } @@ -97,6 +98,7 @@ void memcached_result_reset(memcached_result_st *ptr) ptr->item_flags= 0; ptr->item_cas= 0; ptr->item_expiration= 0; + ptr->numeric_value= UINT64_MAX; } void memcached_result_free(memcached_result_st *ptr) @@ -107,6 +109,7 @@ void memcached_result_free(memcached_result_st *ptr) } memcached_string_free(&ptr->value); + ptr->numeric_value= UINT64_MAX; if (memcached_is_allocated(ptr)) { diff --git a/tests/libmemcached-1.0/mem_functions.cc b/tests/libmemcached-1.0/mem_functions.cc index 0256588d..f5545836 100644 --- a/tests/libmemcached-1.0/mem_functions.cc +++ b/tests/libmemcached-1.0/mem_functions.cc @@ -5778,6 +5778,7 @@ test_st replication_tests[]= { {"mget", false, (test_callback_fn*)replication_mget_test }, {"delete", true, (test_callback_fn*)replication_delete_test }, {"rand_mget", false, (test_callback_fn*)replication_randomize_mget_test }, + {"miss", false, (test_callback_fn*)replication_miss_test }, {"fail", false, (test_callback_fn*)replication_randomize_mget_fail_test }, {0, 0, (test_callback_fn*)0} }; diff --git a/tests/libmemcached-1.0/replication.cc b/tests/libmemcached-1.0/replication.cc index be1fd7e2..60c62b85 100644 --- a/tests/libmemcached-1.0/replication.cc +++ b/tests/libmemcached-1.0/replication.cc @@ -327,3 +327,92 @@ test_return_t replication_randomize_mget_fail_test(memcached_st *memc) memcached_free(memc_clone); return TEST_SUCCESS; } + +/* Test that single miss does not cause replica reads to fail */ +test_return_t replication_miss_test(memcached_st *memc) +{ + test_skip(true, false); + + memcached_st *memc_repl= memcached_clone(NULL, memc); + test_true(memc_repl); + memcached_st *memc_single= memcached_clone(NULL, memc); + test_true(memc_single); + + const char *value = "my_value"; + size_t vlen; + uint32_t flags; + + /* this test makes sense only with 2 or more servers */ + test_true(memcached_server_count(memc_repl) > 1); + + /* Consistent hash */ + test_compare(MEMCACHED_SUCCESS, + memcached_behavior_set_distribution(memc_repl, MEMCACHED_DISTRIBUTION_CONSISTENT)); + test_compare(MEMCACHED_SUCCESS, + memcached_behavior_set_distribution(memc_single, MEMCACHED_DISTRIBUTION_CONSISTENT)); + + /* The first clone writes to all servers */ + test_compare(MEMCACHED_SUCCESS, + memcached_behavior_set(memc_repl, MEMCACHED_BEHAVIOR_BINARY_PROTOCOL, true)); + test_compare(MEMCACHED_SUCCESS, + memcached_behavior_set(memc_repl, MEMCACHED_BEHAVIOR_NUMBER_OF_REPLICAS, + memcached_server_count(memc_repl))); + + /* Write to servers */ + { + memcached_return_t rc= memcached_set(memc_repl, + test_literal_param(__func__), + value, strlen(value), + time_t(1200), uint32_t(0)); + test_true(rc == MEMCACHED_SUCCESS or rc == MEMCACHED_BUFFERED); + } + + /* Use the second clone to remove the key from primary server. + This should remove the key from only one server */ + { + memcached_return_t rc= memcached_delete(memc_single, + test_literal_param(__func__), + 0); + test_true(rc == MEMCACHED_SUCCESS or rc == MEMCACHED_BUFFERED); + } + + /* Remove the server where the key was deleted */ + { +#if 0 + memcached_return_t rc; + const memcached_server_st *instance= memcached_server_by_key(memc_single, + test_literal_param(__func__), + &rc); + test_compare(MEMCACHED_SUCCESS, rc); + test_compare(MEMCACHED_SUCCESS, + memcached_server_remove(instance)); +#endif + } + + /* Test that others still have it */ + { + memcached_return_t rc; + char *get_value= memcached_get(memc_single, + test_literal_param(__func__), + &vlen, &flags, &rc); + test_true(rc == MEMCACHED_SUCCESS or rc == MEMCACHED_BUFFERED); + test_true(get_value and strcmp(get_value, value) == 0); + free(get_value); + } + + /* This read should still return the value as we have it on other servers */ + { + memcached_return_t rc; + char *get_value= memcached_get(memc_repl, + test_literal_param(__func__), + &vlen, &flags, &rc); + test_true(rc == MEMCACHED_SUCCESS || rc == MEMCACHED_BUFFERED); + test_true(get_value and strcmp(get_value, value) == 0); + free(get_value); + } + + memcached_free(memc_repl); + memcached_free(memc_single); + + return TEST_SUCCESS; +} diff --git a/tests/replication.h b/tests/replication.h index 1bf4e499..f287a624 100644 --- a/tests/replication.h +++ b/tests/replication.h @@ -37,28 +37,16 @@ #pragma once -#ifdef __cplusplus -extern "C" { -#endif - -LIBTEST_LOCAL test_return_t replication_set_test(memcached_st *memc); -LIBTEST_LOCAL test_return_t replication_get_test(memcached_st *memc); -LIBTEST_LOCAL test_return_t replication_mget_test(memcached_st *memc); -LIBTEST_LOCAL test_return_t replication_delete_test(memcached_st *memc); -LIBTEST_LOCAL test_return_t replication_randomize_mget_test(memcached_st *memc); -LIBTEST_LOCAL test_return_t replication_randomize_mget_fail_test(memcached_st *memc); -#ifdef __cplusplus -} -#endif +test_return_t replication_miss_test(memcached_st *memc); -- 2.30.2