INTERNAL: Move io reset from where calls memcached_vdo() to where fails insied of memcached_vdo() #167 #250

uhm0311 · 2022-04-19T07:53:52Z

memcached_do() 함수 혹은 memcached_vdo() 함수 호출 시 실패하면 io reset하는 부분을 각 함수의 내부로 옮겼습니다.
두 함수를 호출하는 쪽에서 성공 혹은 실패 시 처리에 일관성이 없는 점을 개선했습니다.

SuhwanJang

@uhm0311
리뷰 완료하였습니다. 커밋 오타 (insied => inside)

SuhwanJang · 2022-04-21T09:50:20Z

libmemcached/collection.cc

+  }
+  if (ptr->flags.no_reply || ptr->flags.piped) {
+    return MEMCACHED_SUCCESS;
+  }


memcached_vdo() 수행 후 어떤 조건이면 바로 리턴하도록 수정되었는데요,
리턴 전에 수행해야 할 작업이 있으며, 여기서는 mkey_buffer free 가 해당됩니다.
다른 연산도 빠져 있는 지 확인 바랍니다.

SuhwanJang · 2022-04-21T09:53:52Z

libmemcached/delete.cc

+  {
+    return MEMCACHED_SUCCESS;
+  }
+  if (rc == MEMCACHED_SWITCHOVER or rc == MEMCACHED_REPL_SLAVE)


ENABLE_REPLICATION 코드 태그가 빠져있습니다.

SuhwanJang · 2022-04-21T10:02:30Z

libmemcached/collection.cc

+  }
+
+  /* Fetch results */
+  result = memcached_coll_fetch_result(ptr, result, &rc);


c-client code style 수정 (다른 곳도 수정 필요)
result= memcached_coll_fetch_result(ptr, result, &rc);

uhm0311 · 2022-04-21T10:39:37Z

리뷰 반영했습니다.

SuhwanJang

@uhm0311
2차 리뷰 하였습니다.

SuhwanJang · 2022-04-22T01:19:00Z

libmemcached/collection.cc

+  }
+  if (ptr->flags.no_reply || ptr->flags.piped) {
+    return MEMCACHED_SUCCESS;
+  }


위 두 if 문에서는 mkey_buffer free 안해도 되나요?

uhm0311 · 2022-04-22T01:50:43Z

리뷰 반영했습니다.

SuhwanJang

@uhm0311
리뷰하였습니다.

SuhwanJang · 2022-04-22T02:18:48Z

libmemcached/collection.cc

+  }
+  if (ptr->flags.no_reply || ptr->flags.piped) {
+    rc= MEMCACHED_SUCCESS;
+    goto RETURN;


기존의 goto label 이 소문자(do_action) 이므로, 동일하게 소문자로 쓰면 좋겠네요.
그런데, 한 함수 내에 goto 문이 많아지면 로직이 복잡해지고, 코드 중복이 크지 않으므로 각 return 전에 mkey_buffer free 코드 넣는 게 나은 것 같아요.

uhm0311 · 2022-04-22T02:29:12Z

리뷰 반영했습니다.

SuhwanJang · 2022-04-22T09:35:58Z

libmemcached/collection.cc

+      mkey_buffer= NULL;
+    }
+    return MEMCACHED_SUCCESS;
+  }


@uhm0311
코드를 아래와 같이 작성하는 건 어떤가요?

rc= memcached_vdo(instance, vector, veclen, to_write); #ifdef MEMCACHED_VDO_ERROR_HANDLING if (rc == MEMCACHED_SUCCESS && to_write == false) { rc= MEMCACHED_BUFFERED; } if (rc != MEMCACHED_SUCCESS || ptr->flags.no_reply || ptr->flags.piped) { if (mkey_buffer) { libmemcached_free(ptr, mkey_buffer); mkey_buffer= NULL; } return rc; } /* Fetch results */ result= memcached_coll_fetch_result(ptr, result, &rc); ...

uhm0311 · 2022-04-22T09:59:15Z

리뷰 반영했습니다.

SuhwanJang

@uhm0311 @jhpark816
리뷰 완료하였습니다.

jhpark816

리뷰 완료.

이번 PR에서는 아래 변경으로 한정하시죠.

io reset을 vdo() and do() 함수 내부에서 수행
vdo() and do() 함수의 실패 여부는 memcached_failed() 함수 대신에 return 값을 MEMCACHED_SUCCESS 값과 직접 비교하여 판단.

jhpark816 · 2022-04-26T01:32:34Z

libmemcached/storage.cc

@@ -389,6 +422,7 @@ static memcached_return_t memcached_send_ascii(memcached_st *ptr,

  if (rc == MEMCACHED_WRITE_FAILURE)
    memcached_io_reset(instance);
+#endif


기존의 아래 형식의 코드는 그대로 유지하는 것이 좋겠습니다.
기존 코드가 보기에 편한 것 같고,
vdo 함수의 오류 처리는 향후에 어떤 형태의 코드로 구현하자로
논의하고 나서 한꺼번에 수정하는 것이 좋겠습니다.

if (rc == MEMCACHED_SUCCESS) { . . . }

uhm0311 · 2022-04-26T03:10:22Z

리뷰 반영했습니다.

jhpark816

memcached_vdo()와 memcached_do() 호출하는 모든 코드를 찾아보기 바랍니다.
한두군데 빠진 것 같습니다.

…ls inside of memcached_vdo() naver#167

uhm0311 · 2022-04-26T08:24:52Z

빠뜨린 부분 2곳(memcached_do() 1곳, memcached_vdo() 1곳) 수정했습니다.

jhpark816

review 완료

jhpark816 · 2022-04-26T09:16:51Z

libmemcached/delete.cc

+#ifdef MEMCACHED_VDO_ERROR_HANDLING
+  memcached_return_t rc= memcached_vdo(instance, vector, 3, to_write);
+  if (rc != MEMCACHED_SUCCESS) {
+#else
  memcached_return_t rc= MEMCACHED_SUCCESS;
  if ((rc= memcached_vdo(instance, vector, 3, to_write)) != MEMCACHED_SUCCESS)
  {
    memcached_io_reset(instance);


하나만 더 질문할게요.

기존 코드가 잘못되어 있긴 하지만, memcached_vdo() 수행이 실패하면, 무조건 io_reset 하는 코드도 있습니다.
connect() 하는 과정에서 실패하면 해당 socket은 close 상태로 설정하는 것 같지만,
해당 instance의 state 등(그 외 어떤 필드가 변경되는 지는 잘 모름)은 변경된 상태로 그대로 둡니다.
이러한 정보를 다시 초기화하기 위해서 io_reset 수행이 필요하지 않는가요 ?
초기화하지 않아도 문제가 없는 지를 확인해 주면 좋겠습니다.

uhm0311 requested review from jhpark816 and SuhwanJang April 19, 2022 07:53

SuhwanJang reviewed Apr 21, 2022

View reviewed changes

uhm0311 force-pushed the uhm0311/memcached_vdo_error_handling branch from 90df219 to 8687f56 Compare April 21, 2022 10:33

SuhwanJang reviewed Apr 22, 2022

View reviewed changes

uhm0311 force-pushed the uhm0311/memcached_vdo_error_handling branch from 8687f56 to d822906 Compare April 22, 2022 01:45

SuhwanJang reviewed Apr 22, 2022

View reviewed changes

uhm0311 force-pushed the uhm0311/memcached_vdo_error_handling branch from d822906 to 84ef234 Compare April 22, 2022 02:25

SuhwanJang reviewed Apr 22, 2022

View reviewed changes

uhm0311 force-pushed the uhm0311/memcached_vdo_error_handling branch 2 times, most recently from ea22fd9 to 263a0fb Compare April 22, 2022 09:58

SuhwanJang reviewed Apr 25, 2022

View reviewed changes

jhpark816 reviewed Apr 26, 2022

View reviewed changes

uhm0311 force-pushed the uhm0311/memcached_vdo_error_handling branch 2 times, most recently from 9adb488 to 4252b75 Compare April 26, 2022 03:07

jhpark816 reviewed Apr 26, 2022

View reviewed changes

INTERNAL: Move io reset from where calls memcached_vdo() to where fai…

0cf38da

…ls inside of memcached_vdo() naver#167

uhm0311 force-pushed the uhm0311/memcached_vdo_error_handling branch from 4252b75 to 0cf38da Compare April 26, 2022 08:16

jhpark816 reviewed Apr 26, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTERNAL: Move io reset from where calls memcached_vdo() to where fails insied of memcached_vdo() #167 #250

INTERNAL: Move io reset from where calls memcached_vdo() to where fails insied of memcached_vdo() #167 #250

uhm0311 commented Apr 19, 2022

SuhwanJang left a comment

SuhwanJang Apr 21, 2022

SuhwanJang Apr 21, 2022

SuhwanJang Apr 21, 2022

uhm0311 commented Apr 21, 2022

SuhwanJang left a comment

SuhwanJang Apr 22, 2022

uhm0311 commented Apr 22, 2022

SuhwanJang left a comment

SuhwanJang Apr 22, 2022

uhm0311 commented Apr 22, 2022

SuhwanJang Apr 22, 2022 •

edited

Loading

uhm0311 commented Apr 22, 2022

SuhwanJang left a comment

jhpark816 left a comment •

edited

Loading

jhpark816 Apr 26, 2022 •

edited

Loading

uhm0311 commented Apr 26, 2022

jhpark816 left a comment

uhm0311 commented Apr 26, 2022

jhpark816 left a comment

jhpark816 Apr 26, 2022

uhm0311 Apr 26, 2022

INTERNAL: Move io reset from where calls memcached_vdo() to where fails insied of memcached_vdo() #167 #250

INTERNAL: Move io reset from where calls memcached_vdo() to where fails insied of memcached_vdo() #167 #250

Conversation

uhm0311 commented Apr 19, 2022

SuhwanJang left a comment

Choose a reason for hiding this comment

SuhwanJang Apr 21, 2022

Choose a reason for hiding this comment

SuhwanJang Apr 21, 2022

Choose a reason for hiding this comment

SuhwanJang Apr 21, 2022

Choose a reason for hiding this comment

uhm0311 commented Apr 21, 2022

SuhwanJang left a comment

Choose a reason for hiding this comment

SuhwanJang Apr 22, 2022

Choose a reason for hiding this comment

uhm0311 commented Apr 22, 2022

SuhwanJang left a comment

Choose a reason for hiding this comment

SuhwanJang Apr 22, 2022

Choose a reason for hiding this comment

uhm0311 commented Apr 22, 2022

SuhwanJang Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

uhm0311 commented Apr 22, 2022

SuhwanJang left a comment

Choose a reason for hiding this comment

jhpark816 left a comment • edited Loading

Choose a reason for hiding this comment

jhpark816 Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

uhm0311 commented Apr 26, 2022

jhpark816 left a comment

Choose a reason for hiding this comment

uhm0311 commented Apr 26, 2022

jhpark816 left a comment

Choose a reason for hiding this comment

jhpark816 Apr 26, 2022

Choose a reason for hiding this comment

uhm0311 Apr 26, 2022

SuhwanJang Apr 22, 2022 •

edited

Loading

jhpark816 left a comment •

edited

Loading

jhpark816 Apr 26, 2022 •

edited

Loading