INTERNAL: Call memcached_set_error() when version operation is failed on connect.

[uhm0311]

• 에러 코드 MEMCACHED_VERSION_FAILURE를 추가했습니다.
• memcached_connect() 함수 내에서 memcached_version_instance() 함수가 실패했을 때 memcached_set_error() 함수를 호출하면서 MEMCACHED_VERSION_FAILURE를 사용합니다.
• 에러 코드를 추가한 의도는 connection이 수립 되기 전에 발생한 에러와 connection이 수립 되었으나 version을 가져오는데 실패한 경우의 에러를 분리하기 위함입니다.
• 이러한 분리를 통해 memcached_connect() 함수에서 connection 실패 시에 발생하는 에러 코드와 memcached_version_instance() 실패 시에 발생하는 에러 코드가 중복되어도 어느 쪽에서 문제가 생긴 것인지 알 수 있습니다.

[jhpark816]

리뷰 완료.

memcached_version_instance() 함수에 대해 검증하고 나서, 본 PR 처리하겠습니다.

[ing-eoking]

memcached_version_instance 함수 또한 리턴 타입이
memcached_return_t 이므로
memcached_version_instance 함수에서 MEMCACHED_VERSION_FAILURE을
리턴하는 것이 낫지 않나요?

[ing-eoking]

memcached_return_t memcached_version_instance(memcached_server_st *instance)
{
  if (instance && instance->root)
  {
    if (instance->root->flags.use_udp)
    {
      return MEMCACHED_NOT_SUPPORTED;
    }
    if (instance->root->flags.binary_protocol)
    {
      return version_binary_instance(instance);
    }
    return version_ascii_instance(instance);
  }

  return MEMCACHED_INVALID_ARGUMENTS;
}

현재 memcached_version_instance 함수를 호출하고 있는 쪽은
memcached_connect 하나 뿐이며, 반환한다면 MEMCACHED_INVALID_ARGUMENTS 대신
MEMCACHED_VERSION_FAILURE 호출하는 편이 낫다고 생각됩니다.

[uhm0311]

server 객체에 에러 메세지와 같이 에러에 관한 정보를 설정하는 함수가 memcached_set_error() 함수입니다.
MEMCACHED_VERSION_FAILURE 에러에 관한 정보를 server 객체에 저장하여 사용자가 MEMCACHED_VERSION_FAILURE 에러를 받았을 때 이에 관한 정보를 server 객체를 통해 확인하도록 하기 위함입니다.

[ing-eoking]

      rc= memcached_connect(instance);
      if (memcached_failed(rc))
      {
        memcached_set_error(*instance, rc, MEMCACHED_AT);
        hosts_failed[serverkey]= true;
        continue;
      }

그 경우 memcached_connect 함수 호출 이후 위의 로직을 부르고 있는데,
이 때에 set_error가 되지 않나요?

[uhm0311]

memcached_connect() 함수를 호출하는 부분이 거기에만 있는 것이 아니라서요.

[ing-eoking]

제가 확인한 부분에서는 memcached_connect를 사용하는 방식은
앞의 방식과 아래의 방식을 사용합니다.

 if (memcached_failed(memcached_connect(instance)))
  return 0;

자세한 에러 원인을 설정하지 않더라도 적어도 fail에 대해서 처리를 하고 있는데,
혹시 다른 사용 방식이 있나요?

[ing-eoking]

해당 PR에 대한 제 생각과 @uhm0311 님과 offline으로 들었던 내용 정리입니다.
듣고 이해했던 내용을 토대로 정리했으며, 잘못된 부분이 있을 시 말씀해주시면 감사하겠습니다.

저는 Version_failure를 설정한다면, connect 쪽보다는
version 처리 함수(memcached_version_instance ,version_ascii_instance, version_ascii_instance)
에서 하는 편이 낫다고 생각됩니다.

그리고 이에 대한 set_error의 경우 드는 생각은 다음 2가지 입니다.

1. set_error를 하지 않고, MEMCACHED_VERSION_FAILURE만 리턴
   • arcus-c-client에서 memcached_connect를 사용하고 있는 방식을 보면 다음의 2가지 방식으로 활용하고 있습니다.
     • Fail만 검사

     if (memcached_failed(memcached_connect(instance)))

     • Fail 및 Error 까지 설정

      rc= memcached_connect(instance);
      if (memcached_failed(rc))
      {
        memcached_set_error(*instance, rc, MEMCACHED_AT);
      }

     • Fail만 검사하는 로직의 함수가 memcached_behavior_get, memcached_do, memcached_vdo, binary_mget 이기에 오히려 해당 상황에서 Error 설정을 안하게끔 의도된 것이 아닐까 생각이 듭니다. (확실하지 않습니다)
2. version 처리 함수에서 set_error를 수행
   • Version Failure에 대한 set_error가 무조건 이루어져야 할 경우입니다.
   • 좀 더 자세한 에러 핸들링이 있는 편이 낫다고 생각됩니다.
   • set_error를 통해 추가된 에러는 링크드 리스트 형태로 관리하고 있습니다.
   • version 처리 함수 로직을 자세히 보진 못했으나 간단히 봤을 때는 가능할 것으로 보입니다.

궁금한 점

• memcached_version_instance(server)에서 발생되는 에러넘버가 소켓 write / read 에러로 보입니다.
  • 이 부분은 단지 version이라는 명령어를 보냈으나 write / read 가 실패한 것인데, 따로 version failure를 설정할 이유가 있나요?
  • 또한 memcached_connect에서 memcached_version_instance 함수를 제외하고, write / read 실패 상황이 있나요?
• 사용자에게 리턴값으로 MEMCACHED_SUCCESS 혹은 에러넘버를 받게되는데, set_error가 필요한 이유는 좀 더 자세한 에러 과정을 보여주기 위함인가요? 아니면 내부 처리과정에서 에러 넘버가 덮어쓰기가 되는 상황이 발생하기 때문인가요?
  • 에러 추가 상황에서 중복적으로 에러가 들어가는 상황이 발생할 수 있나요?
    • ex) Write Error -> Version Fail -> Write Error

[uhm0311]

따로 version failure를 설정할 이유가 있나요?

• 첫번째 코멘트의 PR 의도를 참조 바랍니다.

에러 추가 상황에서 중복적으로 에러가 들어가는 상황이 발생할 수 있나요?

해당 경우를 상정한 것이 맞습니다.

[ing-eoking]

첫번째 코멘트의 PR 의도를 참조 바랍니다

에러 코드를 추가한 의도는 connection이 수립 되기 전에 발생한 에러와 connection이 수립 되었으나 version을 가져오는데 실패한 경우의 에러를 분리하기 위함입니다.

연산에서 연산 명령어 보내기 전 memcached_connect를 통해 version 명령어를 보내는데, 해당 연산의 write/read 결과 실패와 version 명령어 write/read 결과 실패를 나누어야 하는 이유에 대해 궁금했습니다;;

그리고 memcached_connect 는 외부 API로 사용하나요?

• 에러 추가 상황에서 중복적으로 에러가 들어가는 상황이 발생할 수 있나요?
  해당 경우를 상정한 것이 맞습니다.

이 말은 중복으로 들어가게끔 만든 것이라는 말인가요? 아니면 중복으로 안 들어가게끔 구현한 것이라는 말인가요?
후자라면, memcached_connect에 대한 Fail 및 Error 까지 설정하는 경우 리턴값 또한 set_error를 하게 되어
Write Error -> Version Fail -> Write Error 상황이 만들어지지 않나요?

[uhm0311]

연산에서 연산 명령어 보내기 전 memcached_connect를 통해 version 명령어를 보내는데, 해당 연산의 write/read 결과 실패와 version 명령어 write/read 결과 실패를 나누어야 하는 이유

저희가 디버그 하기에 더 용이해지고 사용자가 더 자세한 에러 원인을 알 수 있습니다.

memcached_connect 는 외부 API로 사용하나요?

그렇진 않습니다.

사용자에게 리턴값으로 MEMCACHED_SUCCESS 혹은 에러넘버를 받게되는데, set_error가 필요한 이유는 좀 더 자세한 에러 과정을 보여주기 위함인가요? 아니면 내부 처리과정에서 에러 넘버가 덮어쓰기가 되는 상황이 발생하기 때문인가요?

내부 처리과정에서 에러 넘버가 덮어쓰기도 있을 수 있습니다.
저희가 디버그 하거나 사용자가 좀 더 자세한 에러 과정을 알면 원인 추적에 더 도움이 될 것입니다.

Write Error -> Version Fail -> Write Error

1개의 API 내에서 동일한 에러가 연속으로 발생하진 않을 것으로 보입니다.
하지만 1개의 API에서 동일한 에러가 set_error()되더라도 error_messages의 순서를 보면 어디에서 어떤 순서로 에러가 발생했는지 추적하기 쉬울 것입니다.

[namsic]

정확히 어느 부분에서 정리가 필요한지는 잘 이해하지 못했는데, 우선은 제 리뷰 의견을 추가하겠습니다.

이러한 분리를 통해 memcached_connect() 함수에서 connection 실패 시에 발생하는 에러 코드와 memcached_version_instance() 실패 시에 발생하는 에러 코드가 중복되어도 어느 쪽에서 문제가 생긴 것인지 알 수 있습니다.

에러 코드 추가 없이 memcached_set_error()만 추가로 호출하는 구현은 어떤가요?

  if (memcached_success(rc))
  {
    memcached_mark_server_as_clean(server);
    rc= memcached_version_instance(server);
-   return rc;
+   return memcached_set_error(*server, rc, MEMCACHED_AT);
  }

• memcached_version_instance()는 문제 원인에 따라 리턴하는 에러 코드가 달라지는데,
  이를 MEMCACHED_VERSION_FAILURE로 덮어쓰게 되면 오히려 세부 원인 파악은 더 어려워지는게 아닌가 생각합니다.

• MEMCACHED_AT은 파일명과 코드 line에 대한 정보를 포함하기 때문에, 에러 코드가 중복되어도 메시지로 문제 발생 시점을 구분 가능할 것으로 생각합니다.

• 한 곳에서만 사용하기 위한 용도로 에러 코드를 추가하는 것이 조금 걸립니다.

[ing-eoking]

@namsic

memcached_version_instance()는 문제 원인에 따라 리턴하는 에러 코드가 달라지는데,

리턴하는 에러 코드는 덮어쓰기가 되고있는 상황이라면 가장 상위의 에러를 반환하는 것이 맞지 않나요?
세부 원인 파악은 내부적으로 필요시 set_error로 이루어져야 할 것이고요.

MEMCACHED_AT은 파일명과 코드 line에 대한 정보를 포함하기 때문에, 에러 코드가 중복되어도 메시지로 문제 발생 시점을 구분 가능할 것으로 생각합니다.

arcus_vdo 에서는 memcached_connect를 다시 부릅니다. 이때에도 발생 코드 라인만으로도 중복 구분이 용이해지나요?

@uhm0311
PR과 별개로 궁금한 사항으로는 혹시 version failure라는 에러가 디버깅에 중요한 지표가 되나요?

[uhm0311]

@ing-eoking

PR과 별개로 궁금한 사항으로는 혹시 version failure라는 에러가 디버깅에 중요한 지표가 되나요?

API 호출 시 아래의 에러들을 구분하는 것이 의미가 있다고 생각하고 PR을 올렸던 것으로 기억합니다.

1. 연결 시도가 실패한 것인지
2. 연결은 수립되었으나 version 명령어가 실패한 것인지
3. 연결과 version 명령어는 성공했으나 사용자가 요청한 연산이 실패한 것인지

[namsic]

@ing-eoking

어떤 부분을 이야기하고자 하는 것인지 잘 모르겠는데, 본인이 생각하는 맞는 구현 방향을 정리해서 올려 주면 좋겠습니다.

현재 memcached_version_instance 함수를 호출하고 있는 쪽은 memcached_connect 하나 뿐이며, 반환한다면 MEMCACHED_INVALID_ARGUMENTS 대신 MEMCACHED_VERSION_FAILURE 호출하는 편이 낫다고 생각됩니다.

memcached_version_instance 함수 또한 리턴 타입이 memcached_return_t 이므로 memcached_version_instance 함수에서 MEMCACHED_VERSION_FAILURE을 리턴하는 것이 낫지 않나요?

위 코멘트에 따르면

• memcached_connect()에서 set_error() 등을 호출하는 대신 기존 구현을 유지하고
• memcached_version_instance()의 구현만 아래와 같이 변경해야 한다는 이야기인가요?

memcached_return_t memcached_version_instance(memcached_server_st *instance)
{
  memcached_return_t rc;
  if (instance && instance->root)
  {
    if (instance->root->flags.use_udp)
    {
-     return MEMCACHED_NOT_SUPPORTED;
+     return MEMCACHED_VERSION_FAILURE
    }
    if (instance->root->flags.binary_protocol)
    {
-     return version_binary_instance(instance);
+     rc =version_binary_instance(instance);
+     if (memcached_failed(rc)) {
+       return MEMCACHED_VERSION_FAILURE;
+     }
+     return rc;
    }
-   return version_ascii_instance(instance);
+   rc = version_ascii_instance(instance);
+   if (memcached_failed(rc)) {
+     return MEMCACHED_VERSION_FAILURE;
+   }
+   return rc;
  }

- return MEMCACHED_INVALID_ARGUMENTS;
+ return MEMCACHED_VERSION_FAILURE
}

[ing-eoking]

@namsic

memcached_connect()에서 set_error() 등을 호출하는 대신 기존 구현을 유지하고

set_error()의 경우, 언급했다시피 MEMCACHED_AT을 넣어 소스코드 상의 위치를 알려주는 경우가 있습니다.
바이너리 파일을 사용하는 사용자 입장으로 볼 때, set_error()는 개발자를 위한 디버깅 목적으로 판단됩니다.
물론 추가하여 디버깅 이점을 가져온다면 추가하는 것이 좋다고 생각되지만

set_error()를 connect 는 성공했으나 version 명령어 write/read 실패에 대하여
디버깅을 추가하는 것이, 그리고 memcached_connect에서 설정하는 것이 항상 이점을 가져올 지에 대하여 잘 판단이 되진 않습니다.

memcached_version_instance()의 구현만 아래와 같이 변경해야 한다는 이야기인가요?

올려주신 코드에서 모든 걸 VERSION_FAILURE로 바꾸는 것이 아니라 아래가 나을 것이라 생각됩니다.

memcached_return_t memcached_version_instance(memcached_server_st *instance)
{
  memcached_return_t rc;
  if (instance && instance->root)
  {
    if (instance->root->flags.use_udp)
    {
      return MEMCACHED_NOT_SUPPORTED;
    }
    if (instance->root->flags.binary_protocol)
    {
-      return version_binary_instance(instance);
+      rc =version_binary_instance(instance);
    }
-   return version_ascii_instance(instance);
+   else
+   {
+      rc = version_ascii_instance(instance);
+   }
+   if (memcached_failed(rc)) {
+     return MEMCACHED_VERSION_FAILURE;
+   }
+   return rc;
  }

return MEMCACHED_INVALID_ARGUMENTS;
}

하지만 memcached_connect 에서 set_error() 및 리턴값을 처리한다면, 상위 에러인 VERSION_FAILURE 쪽으로가 맞다고 생각됩니다.

[namsic]

현재 기준 제 의견을 정리합니다. @uhm0311 최종 정리해 주시면 좋겠습니다.
아래 내용 외에 제 의견이 더 필요한 부분이 있으면 알려주시기 바랍니다.

• 문제 발생 시 디버깅을 위해 에러 발생 위치를 구분할 수 있도록 하는 것은 적절해 보입니다.
• 다만 한 곳에서만 사용하기 위한 목적으로 에러 코드를 추가하는 것이 조금 걸려서,
  다른 방법(MEMCACHED_AT 등)을 사용한 구분이 가능한지를 확인하고자 했습니다.
• @ing-eoking 어떤 부분에서 더 정리가 필요하다고 생각하는 것인지 잘 이해하지 못했습니다.

[ing-eoking]

제 의견 정리입니다.

• PR에서의 arcus_connect에서 set_error는 connect에 성공했으나 version 명령어 실패에 대한 것입니다.
  • 어째서 connect에서 따로 version 명렁어를 보내는 것일까 궁금했습니다.
    • 하위 호환성을 위한(바뀌거나 존재하지 않을 수 있는 명령어 mget, smget 등등) 것이 아닐까 생각됩니다.
    • version_instance에서 version 정보를 받아와 parsing하고, 확인하는 로직이 있는지 궁금했습니다.
      • 추후에 확인 예정
    • 다른 케이스(단순히 K/V get을 쓰는 상황 등)를 생각해봤을 때는 디버깅에 도움을 주는지는 잘 모르겠습니다.
  • 간단히 살펴봤을 때, 아래와 같은 상황에서 arcus_connect 실패에 대해 외부에서 set_error를 따로 설정합니다.
    • arcus_connect Fail에 대하여 외부에서 set_error를 설정하고 있는 명령어
      • do_coll_get
      • do_coll_mget
      • do_bop_smget
    • arcus_connect Fail에 대하여 외부에서 설정하고 있지 않은 명령어
      • memcached_mget_by_key
• VERSION_FAILURE 라는 에러를 두는 것은 좋다고 생각됩니다.
  • 이 에러를 지금의 PR과 같은 상황에서 하는게 맞는지, 좀 더 세분화된 특정 상황으로 해야 할지는 아직 모르겠습니다.

[uhm0311]

@jhpark816 @namsic @ing-eoking

본 PR이 급한 변경 사항은 아닌 것 같은데, 모두 동의한다면 Close 하고 이슈로 올려두어도 될까요?
추후에 본 PR과 같은 변경사항이 필요하다면 이슈와 본 PR에서 논의한 사항들을 참고하면 될 것 같습니다.

[jhpark816]

정리 의견을 전달합니다.

• MEMCACHED_AT 정보로 오류 발생 위치를 알 수 있으므로, 새로운 오류는 정의하지 않도록 합시다.
• 본 PR과 같이 version 연산 실패 시에 오류 설정하는 코드만 추가하시죠.
  • 이 부분만 추가하면 version 연산에서 실패하였음을 확인할 수 있습니다.

그 외의 여러 문의에 대해 설명하면,

• 기존에 오류 발생 위치를 확인할 수 없어 원인 분석이 어려웠던 구체적 사항을 이슈를 올린 후에 본 PR이 나왔으면
  본 PR 필요성에 대한 질문이 자연스럽게 해소될 것입니다.
  • 하지만, 현재 PR 작성자가 오래된 이슈라 구체적 사항을 기억하지 못한 상태이고,
    단지 오류 발생 위치의 확인 필요성에 의해 PR 올린 것임을 감안해 주면 됩니다.
• 오류를 set하는 기존 코드는 일관성 관점에서 점검하고 리팩토링이 필요해 보입니다.
  따라서, 본 PR에 보인 version 연산 실패 코드에서 오류를 설정하는 것이 큰 문제는 없습니다.

[uhm0311]

@jhpark816

본 PR과 같이 version 연산 실패 시에 오류 설정하는 코드만 추가하시죠.

5716509
위 변경사항을 말씀하시는 걸까요? 아니면 현재 변경사항을 말씀하시는 걸까요?

[jhpark816]

@uhm0311

아래 코멘트에 있는 코드를 확인하세요.
#245 (review)

[uhm0311]

@jhpark816

반영했습니다.