INTERNAL: Refactor a memcached_fetch function

[ing-eoking]

🔗 Related Issue

• INTERNAL: Remove dummy fetch in the get API #334

⌨️ What I did

• memcached_fetch 함수가 리팩토링되었습니다.
• use_udp 부분을 확인한 결과, 필요하지 않아서 제거했습니다.
• if (result_buffer == NULL || memcached_failed(*error)) 조건문에서 memcached_failed를 제거했습니다.
  • memcached_fetch_result에서 error 값이 memcached_success가 아닌 경우, 항상 NULL을 반환하도록 되어 있습니다.
• libmemcached를 확인한 결과, 별다른 변경 사항은 없는 것으로 보입니다.
  • libmemcached/src/libmemcached/fetch.cc

[jhpark816]

리뷰 완료

[jhpark816]

아래와 같이 합시다.

if (result_buffer != NULL) *error= MEMCACHED_KEY_TOO_BIG;

[ing-eoking]

수정되었습니다.

[jhpark816]

result_buffer != NULL이지만, *error 값이 failture인 경우는 없다는 것이죠?

memcached_fetch_result() 함수에서
result_buffer != NULL이면서 result_buffer->key_length > MEMCACHED_MAX_KEY 경우
MEMCACHED_KEY_TOO_BIG 에러 설정하여 리턴하려는 의도가 있었던 것이 아닌지 ?

[ing-eoking]

의도까지는 모르겠으나 memcached_fetch_result에서 MEMCACHED_MAX_KEY를 반환하는 형태가 더 자연스러운 것 같습니다.

[jhpark816]

@ing-eoking
논리적으로 봐서,
result_buffer->key_length > MEMCACHED_MAX_KEY 인 경우 오류를 리턴할 필요는 없어 보입니다.

특히나, memcached_fetch()에서는 key에 대해 관심이 없고 단지 value만 조회하는 경우입니다.
이러한 조건 검사가 들어간 이유를 간단히 확인해 볼 수 있을까요?

[ing-eoking]

@jhpark816

libmemcached 커밋 메시지를 확인한 결과, 따로 이유를 적어 놓지는 않았습니다.
awesomized/libmemcached#7e592f0

We now check return key size for memcached_fetch() to make sure it is not
too big (compared to original ascii server). result_st is now initialized 
differently. This should make it a bit faster.

하지만 result_buffer->key_length > MEMCACHED_MAX_KEY 비교를 memcached_fetch나 memcached_fetch_result에서 하는 것은 다소 부자연스러워 보입니다.

result_buffer 구조체 내의 item_key 변수가 이미 MEMCACHED_MAX_KEY 크기로 고정되어 있습니다.
키를 저장할 때 오버플로우가 발생한 경우 이를 처리하는 방식이 더 적합하지 않을까 생각됩니다.

struct memcached_result_st {
  ...
  size_t key_length;
  ...
  char item_key[MEMCACHED_MAX_KEY];
  ...
};

[jhpark816]

리뷰 완료

[jhpark816]

result가 존재할 경우는 위에서 이미 return 하기 때문에, 이 코드가 수행될 수가 없습니다.

[jhpark816]

memcached_fetch_result() 호출하는 경우는 memcached_fetch() 외에 다른 경우도 있습니다.

MEMCACHED_KEY_TOO_BIG 오류를 리턴하게 만들면, 다른 호출 경우에도 문제가 없다는 것을 확인한 상태인가요?
확인하기 어렵다면, 현재로는 처음 PR 처럼 memcached_fetch()에서만 MEMCACHED_KEY_TOO_BIG 오류를 설정하게 하는 것도 괜찮을 듯 합니다.

[ing-eoking]

우선 이전 PR로 복구했습니다.

MEMCACHED_KEY_TOO_BIG 오류를 리턴하게 만들면, 다른 호출 경우에도 문제가 없다는 것을 확인한 상태인가요?

따로 확인은 하진 않았습니다. 다만, memcached_fetch_result()가 공개된 API라 내부에서 어떻게 사용되는지 확인한다고 해서 문제가 없다고 단정할 수는 없을 것 같습니다.