memcached_get_by_key() 함수 마지막에 dummy fetch 필요성 검토.

[jhpark816]

memcached_get_by_key() 함수의 마지막에 아래와 같은 dummy fetch 코드가 있다.
필요성을 검토해 보고, 필요가 없다면 제거합시다.

  size_t dummy_length;
  uint32_t dummy_flags;
  memcached_return_t dummy_error;

  char *dummy_value= memcached_fetch(ptr, NULL, NULL,
                                     &dummy_length, &dummy_flags,
                                     &dummy_error);
  assert_msg(dummy_value == 0, "memcached_fetch() returned additional values beyond the single get it expected");
  assert_msg(dummy_length == 0, "memcached_fetch() returned additional values beyond the single get it expected");
  assert_msg(ptr->query_id >= query_id +1, "Programmer error, the query_id was not incremented.");
  //assert_msg(ptr->query_id == query_id +1, "Programmer error, the query_id was not incremented.");

[jhpark816]

dummy fetch 기능이 불필요하여 제거하였습니다.

• commit) 7b5cf92

[jhpark816]

dummy fetch 제거하면 unit test 실패합니다.
이에 관한 @uhm0311 코멘트는 아래와 같습니다.

---

commit: 7b5cf92
해당 커밋으로 유닛 테스트 실패가 생겼습니다.

tests/mem_functions.cc:4309: Assertion "no_msg == 0" failed, in noreply_test
	Testing noreply					[ failed ]

해당 테스트 코드는 다음과 같습니다.

int no_msg=0;
for (uint32_t x= 0; x < memcached_server_count(memc); ++x)
{
  memcached_server_instance_st instance=
    memcached_server_instance_by_position(memc, x);
  no_msg+=(int)(instance->cursor_active);
}

test_true(no_msg == 0);

아래 코드의 제거로 인해 instance->cursor_active 값이 0이 아니게 되었습니다.

char *dummy_value= memcached_fetch(ptr, NULL, NULL,
                                   &dummy_length, &dummy_flags,
                                   &dummy_error);

memcached_fetch() 함수는 다음과 같습니다.

arcus-c-client/libmemcached/fetch.cc

Lines 56 to 137 in 71bcafd

	char *memcached_fetch(memcached_st *ptr, char *key, size_t *key_length,
	size_t *value_length,
	uint32_t *flags,
	memcached_return_t *error)
	{
	memcached_result_st *result_buffer= &ptr->result;
	memcached_return_t unused;
	if (not error)
	error= &unused;
	unlikely (ptr->flags.use_udp)
	{
	if (value_length)
	*value_length= 0;
	if (key_length)
	*key_length= 0;
	if (flags)
	*flags= 0;
	if (key)
	*key= 0;
	*error= MEMCACHED_NOT_SUPPORTED;
	return NULL;
	}
	result_buffer= memcached_fetch_result(ptr, result_buffer, error);
	if (result_buffer == NULL or memcached_failed(*error))
	{
	WATCHPOINT_ASSERT(result_buffer == NULL);
	if (value_length)
	*value_length= 0;
	if (key_length)
	*key_length= 0;
	if (flags)
	*flags= 0;
	if (key)
	*key= 0;
	return NULL;
	}
	if (value_length)
	{
	*value_length= memcached_string_length(&result_buffer->value);
	}
	if (key)
	{
	if (result_buffer->key_length > MEMCACHED_MAX_KEY)
	{
	*error= MEMCACHED_KEY_TOO_BIG;
	if (value_length)
	*value_length= 0;
	if (key_length)
	*key_length= 0;
	if (flags)
	*flags= 0;
	if (key)
	*key= 0;
	return NULL;
	}
	strncpy(key, result_buffer->item_key, result_buffer->key_length); // For the binary protocol we will cut off the key :(
	if (key_length)
	*key_length= result_buffer->key_length;
	}
	if (flags)
	*flags= result_buffer->item_flags;
	return memcached_string_take_value(&result_buffer->value);
	}

85번 라인에서 호출하는 memcached_fetch_result() 함수는 다음과 같습니다.

arcus-c-client/libmemcached/fetch.cc

Lines 139 to 248 in 71bcafd

	memcached_result_st *memcached_fetch_result(memcached_st *ptr,
	memcached_result_st *result,
	memcached_return_t *error)
	{
	memcached_return_t unused;
	if (not error)
	error= &unused;
	if (not ptr)
	{
	*error= MEMCACHED_INVALID_ARGUMENTS;
	return NULL;
	}
	if (ptr->flags.use_udp)
	{
	*error= MEMCACHED_NOT_SUPPORTED;
	return NULL;
	}
	if (not result)
	{
	// If we have already initialized (ie it is in use) our internal, we
	// create one.
	if (memcached_is_initialized(&ptr->result))
	{
	if (not (result= memcached_result_create(ptr, NULL)))
	{
	*error= MEMCACHED_MEMORY_ALLOCATION_FAILURE;
	return NULL;
	}
	}
	else
	{
	result= memcached_result_create(ptr, &ptr->result);
	}
	}
	*error= MEMCACHED_MAXIMUM_RETURN; // We use this to see if we ever go into the loop
	memcached_server_st *server;
	bool connection_failures= false;
	while ((server= memcached_io_get_readable_server(ptr)))
	{
	char buffer[MEMCACHED_DEFAULT_COMMAND_SIZE + MEMCACHED_MAX_KEY];
	*error= memcached_response(server, buffer, sizeof(buffer), result);
	if (*error == MEMCACHED_IN_PROGRESS)
	{
	continue;
	}
	else if (*error == MEMCACHED_CONNECTION_FAILURE)
	{
	connection_failures= true;
	continue;
	}
	else if (*error == MEMCACHED_SUCCESS)
	{
	result->count++;
	return result;
	}
	else if (*error == MEMCACHED_END)
	{
	memcached_server_response_reset(server);
	}
	else if (*error != MEMCACHED_NOTFOUND)
	{
	break;
	}
	}
	memcached_set_last_response_code(ptr, *error);
	if (*error == MEMCACHED_NOTFOUND and result->count)
	{
	*error= MEMCACHED_END;
	}
	else if (*error == MEMCACHED_MAXIMUM_RETURN and result->count)
	{
	*error= MEMCACHED_END;
	}
	else if (*error == MEMCACHED_MAXIMUM_RETURN) // while() loop was never entered
	{
	*error= MEMCACHED_NOTFOUND;
	}
	else if (connection_failures)
	{
	/* If we have a connection failure to some servers, the caller may
	* wish to treat that differently to getting a definitive NOT_FOUND
	* from all servers, so return MEMCACHED_CONNECTION_FAILURE to allow that.
	*/
	*error= MEMCACHED_CONNECTION_FAILURE;
	}
	else if (result->count == 0)
	{
	*error= MEMCACHED_NOTFOUND;
	}
	/* We have completed reading data */
	if (memcached_is_allocated(result))
	{
	memcached_result_free(result);
	}
	else
	{
	result->count= 0;
	memcached_string_reset(&result->value);
	}
	return NULL;
	}

201번 라인에서 호출하는 memcached_server_response_reset() 함수는 다음과 같습니다.

arcus-c-client/libmemcached/common.h

Line 174 in 71bcafd

#define memcached_server_response_reset(A) (A)->cursor_active=0

따라서 memcached_fetch() 함수를 호출하는 부분은 남겨야 합니다.

[jhpark816]

@uhm0311 의 추가 코멘트 입니다.

---

libmemcached 1.0 버전을 살펴보니, get.cc에서 dummy fetch를 제거한 뒤 문제 되는 테스트 코드를 #if 0으로 비활성화 해뒀습니다.

#if 0
    int no_msg=0;
    for (uint32_t x= 0; x < memcached_server_count(memc); ++x)
    {
      const memcached_instance_st * instance=
        memcached_server_instance_by_position(memc, x);
      no_msg+=(int)(instance->cursor_active);
    }

    test_true(no_msg == 0);
#endif

dummy fetch를 제거하는 방향을 유지하려면 같은 조치를 취해야 할 것 같습니다.

[jhpark816]

@ing-eoking 본 이슈를 맡아 주세요.

[ing-eoking]

dummy fetch가 필요한 이유는 memcached_fetch의 동작 방식 때문입니다.

memcached_fetch는 한 번에 하나의 결과만 가져오기 때문에, 값 조회를 위한 API 호출 시 값(value)과 끝(END)을 가져오기 위한 두 번의 호출이 필요합니다.

이를 해결하기 위해, memcached_get_by_key 내부에서 memcached_fetch의 결과가 MEMCACHED_END일 때까지 반복하도록 변경하면, dummy fetch는 더 이상 필요하지 않게 됩니다.

[jhpark816]

@ing-eoking
OK. 나도 검토하고 의견줄게요.

[jhpark816]

@ing-eoking
Nice한 리팩토링 방안이 있다면, PR 보내 주세요.

[jhpark816]

@ing-eoking
PR 방안은 기존 방안 보다는 Nice한 방안인가요?
그 이유를 간단히 설명해 주세요.

[ing-eoking]

@jhpark816

기존 방식에서는 memcached_fetch를 사용하여 값이 성공적으로 가져왔는지와 관계없이 dummy fetch를 수행했습니다.

반면, 현재 PR에서는 memcached_fetch_result를 사용하며 값을 성공적으로 가져온 경우만 수행됩니다. 예를 들어, 해당 키가 존재하지 않으면 while문은 실행되지 않습니다.

[jhpark816]

기존 방식에서는 memcached_fetch를 사용하여 값이 성공적으로 가져왔는지와 관계없이 dummy fetch를 수행했습니다.

1st memcached_fetch() 결과가 NULL이면, 더 이상 memcached_fetch() 수행하지 않고 있습니다.

[ing-eoking]

1st memcached_fetch() 결과가 NULL이면, 더 이상 memcached_fetch() 수행하지 않고 있습니다.

바로 아래에 로직이 있었네요;

dummy라는 변수를 따로 설정하고 fetch를 수행하지 않기에 기존 로직보다는 비교적 깔끔해진 것 같습니다.