INTERNAL: Do not overwrite a last response on error

[ing-eoking]

🔗 Related Issue

• jam2in/arcus-works#719

⌨️ What I did

• 현재는 collection get에서 에러가 발생할 경우, rc와 last_response가 모두 NOT_FOUND로 덮어써져 정확한 에러 원인을 파악하기 어렵습니다.
• memcached_coll_fetch_result 내부에서 이미 설정된 last_response_code가 덮어써지지 않도록 방지합니다.
• do_coll_get에서는 반환 값을 재설정하는 경우(성공 케이스)에만 last_response_code를 설정하도록 변경했습니다.

[jhpark816]

@ing-eoking
Related Issue에
본 이슈와 관련된 직접적인 내용이 있는 부분의 link를 넣어주세요.

[jhpark816]

• 현재는 collection get에서 에러가 발생할 경우, rc와 last_response가 모두 NOT_FOUND로 덮어써져 정확한 에러 원인을 파악하기 어렵습니다.
• memcached_coll_fetch_result 내부에서 이미 설정된 last_response_code가 덮어써지지 않도록 방지합니다.
• do_coll_get에서는 반환 값을 재설정하는 경우(성공 케이스)에만 last_response_code를 설정하도록 변경했습니다.

last_response는 마지막 response를 설정하여 전달하는 용도라서 당연한 결과가 아닌가 생각됩니다.
본 이슈의 본질적인 문제가 무엇이고, 해당 해결 방안을 다시 정리해 보는 것이 필요한 것 같습니다.

[ing-eoking]

본 이슈와 관련된 직접적인 내용이 있는 부분의 link를 넣어주세요.

직접적으로 관련된 이슈는 없습니다.

last_response는 마지막 response를 설정하여 전달하는 용도라서 당연한 결과가 아닌가 생각됩니다.
본 이슈의 본질적인 문제가 무엇이고, 해당 해결 방안을 다시 정리해 보는 것이 필요한 것 같습니다.

본질적인 문제는 다음과 같습니다.

이미 memcached_coll_fetch_result에서 last_response를 설정한 뒤, rc 값을 리턴했음에도 불구하고 do_coll_get에서 rc 값을 기준으로 다시 last_response를 설정하기 때문에 last_response == rc 가 되고 있습니다.

그렇기 때문에 PR과 같은 변경이 필요하다는 의미입니다.

[jhpark816]

이미 memcached_coll_fetch_result에서 last_response를 설정한 뒤, rc 값을 리턴했음에도 불구하고 do_coll_get에서 rc 값을 기준으로 다시 last_response를 설정하기 때문에 last_response == rc 가 되고 있습니다.

구체적인 예시를 들어 설명해 주면 좋겠습니다.

어떤 collection 조회 연산이고,
memcached_coll_fetch_result 결과는 무엇이었고,
다시 do_coll_get에서 다시 rc 값이 변경되는 것은 무엇 때문인지 등등
(추가 작업을 했으므로 last_response 설정하는 것이 정당한 것이 아닌지)

이를 이슈로 올려 정리하는 것이 좋겠습니다.

[uhm0311]

• https://github.com/jam2in/arcus-works/issues/719#issue-3035015306

위 이슈 설명에서, 아래의 4가지 경우에는 last_response_code를 설정하지 않는다고 했습니다.
그래서 반대로 여기서는 설정해주는 것인가요?

MEMCACHED_SUCCESS
MEMCACHED_TRIMMED
MEMCACHED_DELETED
MEMCACHED_DELETED_DROPPED

[ing-eoking]

네. 맞습니다.

[uhm0311]

MEMCACHED_SUCCESS의 경우에는 last_response_code로 설정하지 않아도 되는건가요?

[ing-eoking]

MEMCACHED_SUCCESS의 경우에는 last_response_code로 설정하지 않아도 되는건가요?

MEMCACHED_SUCCESS의 경우에는 별도로 처리하지 않아도 된다고 생각했지만, last_response 관련 문서를 확인해보니 아래와 같이 설명되어 있어, 4가지 경우(SUCCESS, TRIMMED, DELETED, DELETED_DROPPED)에만 값을 설정하는 것이 더 적절할 것 같습니다.

캐시 명령을 실행한 후에 캐시 서버로부터 받은 응답 코드를 확인할 수 있다.
이 응답코드는 명령의 실행 결과에 대한 추가 정보를 제공한다.

[jhpark816]

PR 코드는
성공할 경우의 response code 설정하는 코드 위치와
실패할 경우의 response code 설정하는 코드 위치가 다릅니다.

두 경우 모두에 대해 하나의 코드 위치에서 response code 설정하면 좋겠습니다.
좋은 방향인가요 그리고 가능한가요?

[ing-eoking]

MEMCACHED_SUCCESS의 경우에는 last_response를 설정하지 않아도 될 것 같습니다.

do_coll_get에서 last_response_code 설정을 제거하는 것이 좋을 것 같습니다.

[jhpark816]

MEMCACHED_SUCCESS 경우에도 specific response가 보관되어 있다면 나을 것입니다.
last_response_code 설정하는 다른 경우에서도 SUCCESS 경우에는 생략되어 있나요?

[ing-eoking]

memcached_fetch_result 의 경우에도 생략되어 있습니다.
그렇다면 fetch 함수 (memcached_fetch_result, memcached_coll_fetch_result)에서 SUCCESS에 대해 last_response를 설정하도록 변경하겠습니다.

[jhpark816]

@ing-eoking
아래 코드에 대해 추가 질문이 있습니다.

memcached_coll_fetch_result() 호출하여 리턴 값이 NULL이 아니면
다시 memcached_coll_fetch_result() 호출합니다. 마지막 END를 읽기 위함인 것으로 보입니다. (??)

memcached_coll_fetch_result() 함수를 1회만 호출하여
그 안에서 마지막 END까지 읽도록 처리가 가능하나요?

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

      /* Search for END or something */
      if (result)
      {
        memcached_coll_result_reset(&ptr->collection_result);
        memcached_coll_fetch_result(ptr, &ptr->collection_result, &rc);
      }

[ing-eoking]

이는 멀티라인 응답 처리 시 필요한 dummy fetch로, Arcus-C-Client 내부 구조상 dummy fetch 호출이 필요하게끔 구현되어 있습니다.
이를 수정하려면 구조 전반에 대한 변경이 필요해 변경 범위가 클 것으로 예상되며, fetch 함수 자체도 외부 API이기 때문에 수정이 어려울 것으로 보입니다.

[jhpark816]

@uhm0311 리뷰 진행하세요.

[jhpark816]

리뷰 완료

[jhpark816]

PR 코드는
성공할 경우의 response code 설정하는 코드 위치와
실패할 경우의 response code 설정하는 코드 위치가 다릅니다.

두 경우 모두에 대해 하나의 코드 위치에서 response code 설정하면 좋겠습니다.
좋은 방향인가요 그리고 가능한가요?