INTERNAL: Remove dummy fetch in the get API #334

ing-eoking · 2025-01-03T08:51:51Z

🔗 Related Issue

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

⌨️ What I did

종료 지점 확인이 비교적 용이한 memcached_fetch_result를 활용했습니다.
- VALUE를 가져오지 못한 경우, NULL이 반환됩니다.
memcached_fetch 내부에서 처리하던 value_length와 flags 초기화 로직을 memcached_get_by_key 함수 초기에 수행하도록 변경했습니다.
~~현재 CentOS7 환경에서 테스트를 진행하는 경우, develop 브랜치에서도 동일하게 아래의 테스트가 실패합니다.~~
- ~~예전 mop_upsert API를 개발하는 과정에서 문제가 있었던 것으로 보이며, 원인 분석중에 있습니다.~~
- 저희 장비에 설치된 memcached가 mop upsert가 없는 구버전이라 생긴 문제였습니다.

tests/mem_functions.cc:11420: in arcus_1_10_map_upsert() Assertation  "rc == MEMCACHED_SUCCESS || rc == MEMCACHED_BUFFERED" failed, hint: CLIENT ERROR
	Testing arcus_1_10_map_upsert					[ failed ]

jhpark816

리뷰 완료

jhpark816 · 2025-01-16T10:41:21Z

libmemcached/get.cc

+      *value_length= memcached_string_length(&result->value);
+    if (flags)
+      *flags= result->item_flags;
+  }


memcached_fetch_result() != NULL 경우가 2회 이상 발생하는 경우도 고려하시죠.

memcached_fetch_result() != NULL 경우가 2회 이상 발생하는 경우도 고려하시죠.

예전에 이렇게 코멘트 했지만, 이러한 경우가 발생하지 않는다고 보는 것이 맞겠죠?
만약의 SW 버그를 고려하더라도 END 이전의 가장 마지막 값을 반환하면 될 것 같습니다.

*error = ENGINE_SUCCESS; while (memcached_fetch_result(ptr, result, error) != NULL) { value= memcached_string_take_value(&result->value); if (value_length) *value_length= memcached_string_length(&result->value); if (flags) *flags= result->item_flags; }

while loop 안에 아래 코드가 있어야 하죠?

if (value) memcached_free(value);

jhpark816 · 2025-01-16T10:42:43Z

libmemcached/get.cc

+  if (success_occurred)
+  {
+    *error= MEMCACHED_SUCCESS;
+  }


success_occurred 경우는 value != NULL 상태입니다.
success_occurred 변수가 필요 없을 것 같습니다.

그리고, 이러한 코드는 value == NULL 경우에 대한 처리 코드 다음에 위치하는 것이 좋겠습니다.

value에 빈 문자열을 저장했을 경우, value == NULL이 될 수 있습니다.

용어를 success_occurred에서 is_value_fetched로 변경했습니다.

@ing-eoking
질문입니다.

value == ""(빈 문자열)을 저장하고 나서, 이를 조회하면 빈 문자열이 리턴되어야 하지 않나요?

value == NULL로 해서 저장할 수 있나요?

result->value에는 빈 문자열이 들어가는데, 이를 다시 value에 copy하여 넣는 과정에서 NULL로 변환됩니다.
memcached_string_take_value 함수 내부 동작에서 문자열의 길이가 0이면 NULL을 반환하도록 되어 있기 때문입니다

if (not memcached_string_length(string)) return NULL;

value == NULL로 해서 저장할 수 있나요?

value == NULL로 해서 저장할 수 있나요?

불가능합니다. 서버 측에서는 value에 NULL 값을 저장할 수 없으며,
클라이언트 측에도 NULL Value를 처리하는 기능이 없습니다.

value = NULL, value_length = 0으로 하여 set 하면 어떤 오류가 발생하나요?

value_length가 0이므로 빈문자열이 들어가게 됩니다.

memcached_set(memc, "i", 1, NULL, 0, 0, 0);

/* telnet */ get i VALUE i 0 0 END

jhpark816 · 2025-02-14T12:19:13Z

@ing-eoking 최신화 해 주세요.

ing-eoking · 2025-02-18T02:25:31Z

@jhpark816

최신화 되었습니다.

jhpark816

리뷰 완료

jhpark816 · 2025-02-18T11:29:42Z

libmemcached/get.cc

+      *value_length= memcached_string_length(&result->value);
+    if (flags)
+      *flags= result->item_flags;
+  }


memcached_fetch_result() != NULL 경우가 2회 이상 발생하는 경우도 고려하시죠.

예전에 이렇게 코멘트 했지만, 이러한 경우가 발생하지 않는다고 보는 것이 맞겠죠?
만약의 SW 버그를 고려하더라도 END 이전의 가장 마지막 값을 반환하면 될 것 같습니다.

*error = ENGINE_SUCCESS; while (memcached_fetch_result(ptr, result, error) != NULL) { value= memcached_string_take_value(&result->value); if (value_length) *value_length= memcached_string_length(&result->value); if (flags) *flags= result->item_flags; }

jhpark816

리뷰 완료

jhpark816 · 2025-02-19T02:45:06Z

libmemcached/get.cc

+      *value_length= memcached_string_length(&result->value);
+    if (flags)
+      *flags= result->item_flags;
+  }


while loop 안에 아래 코드가 있어야 하죠?

if (value) memcached_free(value);

jhpark816 · 2025-02-19T02:46:54Z

libmemcached/get.cc

  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.");

-  /* This is for historical reasons */
-  if (*error == MEMCACHED_END)
+  if (is_value_fetched)


is_value_fetched 라는 변수가 필요한가요?

아래와 같이 검사하면 되지 않나요?

if (value)

ing-eoking · 2025-02-19T03:12:13Z

while loop 안에 아래 코드가 있어야 하죠?

메모리 해제 로직 추가하겠습니다.

is_value_fetched 라는 변수가 필요한가요?

INTERNAL: Remove dummy fetch in the get API #334 (comment)

success인 상황에서도 value가 NULL이 될 수 있어서 였습니다.

ing-eoking force-pushed the dummy branch from f11de11 to a31b8bd Compare January 6, 2025 00:16

jhpark816 requested a review from namsic January 13, 2025 01:33

ing-eoking mentioned this pull request Jan 14, 2025

INTERNAL: Refactor a memcached_fetch function #337

Merged

jhpark816 reviewed Jan 16, 2025

View reviewed changes

ing-eoking force-pushed the dummy branch from a31b8bd to b845424 Compare January 17, 2025 01:18

ing-eoking force-pushed the develop branch from abcfe88 to 672a824 Compare January 24, 2025 06:13

ing-eoking force-pushed the dummy branch from b845424 to 5ecb139 Compare February 17, 2025 00:10

ing-eoking marked this pull request as draft February 17, 2025 02:11

ing-eoking force-pushed the dummy branch 4 times, most recently from 63f1421 to 887b064 Compare February 18, 2025 02:10

ing-eoking marked this pull request as ready for review February 18, 2025 02:25

jhpark816 reviewed Feb 18, 2025

View reviewed changes

ing-eoking force-pushed the dummy branch from 887b064 to 01eeebf Compare February 19, 2025 02:38

jhpark816 reviewed Feb 19, 2025

View reviewed changes

ing-eoking force-pushed the dummy branch from 01eeebf to 8989a3e Compare February 19, 2025 05:35

INTERNAL: Remove dummy fetch in the get API

ca2d0d8

ing-eoking force-pushed the dummy branch from 8989a3e to ca2d0d8 Compare February 19, 2025 05:42

jhpark816 approved these changes Feb 20, 2025

View reviewed changes

jhpark816 merged commit 5055532 into naver:develop Feb 20, 2025
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTERNAL: Remove dummy fetch in the get API #334

INTERNAL: Remove dummy fetch in the get API #334

ing-eoking commented Jan 3, 2025 •

edited

Loading

jhpark816 left a comment

jhpark816 Jan 16, 2025

This comment was marked as off-topic.

jhpark816 Feb 18, 2025

jhpark816 Feb 19, 2025

jhpark816 Jan 16, 2025

ing-eoking Jan 17, 2025 •

edited

Loading

jhpark816 Feb 19, 2025

ing-eoking Feb 19, 2025

jhpark816 Feb 19, 2025

ing-eoking Feb 19, 2025 •

edited

Loading

jhpark816 Feb 19, 2025

ing-eoking Feb 19, 2025

jhpark816 commented Feb 14, 2025

ing-eoking commented Feb 18, 2025

jhpark816 left a comment

jhpark816 Feb 18, 2025

jhpark816 left a comment

jhpark816 Feb 19, 2025

jhpark816 Feb 19, 2025

ing-eoking commented Feb 19, 2025

INTERNAL: Remove dummy fetch in the get API #334

INTERNAL: Remove dummy fetch in the get API #334

Conversation

ing-eoking commented Jan 3, 2025 • edited Loading

🔗 Related Issue

⌨️ What I did

jhpark816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ing-eoking Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ing-eoking Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 commented Feb 14, 2025

ing-eoking commented Feb 18, 2025

jhpark816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ing-eoking commented Feb 19, 2025

ing-eoking commented Jan 3, 2025 •

edited

Loading

ing-eoking Jan 17, 2025 •

edited

Loading

ing-eoking Feb 19, 2025 •

edited

Loading