Wooseok/fix test timeout

[aiceru]

1. timeout test 를 USE_ZK false 일 때만 실행 (USE_ZK 에 관련없이 동일한 로직이므로)
2. fake address node 를 이용하여 확실히 timeout 이 발생함을 보장 (random 하게 build 가 성공/실패 하는 것을 방지)
   • 간혹 testIPv6Host() 테스트가 fail되는 경우가 있습니다. 테스트결과가 일관적이지 않아서 검색해보니, travis 자체의 버그인듯 합니다. 우선, 원래의 코드를 그대로 유지하는 방향으로 하고 추후 문제가 발생할 경우 다시 검토하도록 하겠습니다.

Review 부탁드립니다.

• [x] @whchoi83
• [x] @jhpark816

[jhpark816]

훈민님 의견입니다..

11:00 김훈민 / Hoonmin Kim 말씀해주신 ~~SingleClient() 테스트들 쭉 살펴봤는데요 11:01 김훈민 / Hoonmin Kim 정확한 의도는 잘 모르겠지만 아마도 ArcusClientPool, ArcusClient 두 가지를 각각 테스트하려는 의도로 생각됩니다. 11:04 김훈민 / Hoonmin Kim 저 의도가 맞다면 BaseIntegrationTest나 각 테스트 케이스에 ArcusClientPool 객체를 추가하고 ~SingleClient 가 아닌 테스트는 pool로 테스트하는게 맞을 것 같아요

[aiceru]

말씀하신 대로, single client 를 사용하는 경우와 client pool 을 사용하는 경우를 구분하는 것은 큰 의미는 없는 것 같습니다. ~~~UsingSingleClient 테스트를 삭제하였습니다.

테스트 순서도 collection 별로 정렬하여 수정하였습니다.

bulk set 의 경우는 thread 여러개(6개), 1개 를 사용하는 경우로 나누어, testBulkSetTimeout(), testBulkSetTimeoutUsingSingleThread() 의 2가지 테스트 코드로 작성하였습니다.

이외 다른 부분에서의 (timeout 과 관련없는) ~~~UsingSingleThread() 코드의 중복에 대해서는 별도의 PR 로 작업하도록 하겠습니다.

[whchoi83]

테스트에 사용했던 key 들은 삭제해야 되지 않을까 싶습니다. CI 테스트 경우야 문제가 없지만 local 에서 테스트를 진행하는 경우는 테스트에 사용했던 key 들은 그대로 남아 있습니다. (꼭 이 테스트의 문제가 아니라 테스트 코드의 전체적인 점검이 필요한 상태입니다. teardown 시에 지우는 테스트도 있고 아닌 테스트들도 있는 것 같습니다.)

[aiceru]

이 테스트에서는 key 를 삭제할 필요가 없습니다... 라기보단 삭제를 할 수도 없어요 😞 실제 존재하지 않는 node ("127.0.0.1:64213") 에 request 를 보내고 timeout exception 발생을 확인하는 테스트이기 때문입니다.

teardown 시에 flush 하는 케이스도 있고 아닌 케이스도 있는데, 지금은 그 구분의 이유를 명확히 파악하기가 어렵네요. 똑같은 최상위 base class 라도, 어떤 경우는 shutdown, restart, flush, shutdown 을 하고 있고, 어떤 경우는 또 단순히 shutdown 만 하고 있고 그렇습니다. 아마 코드 형태로 보아 spymemcached -> arcus 로 확장시 테스트 케이스를 작성하면서 필요에 의해 넣은 것 같긴 합니다. 좀 더 코드를 살펴보고 필요하다고 판단될 시 별도의 이슈로 등록하여 진행하도록 하겠습니다.

[whchoi83]

@aiceru 아. 그렇네요. 바보 같은 착각을 했습니다. 😅

이 외의 테스트는 별도의 이슈로 처리하면 될 것 같습니다.

[whchoi83]

리뷰 완료했습니다. 최신 develop branch 내용만 반영해주시면 될 것 같습니다.

[jhpark816]

질문이 있어요

• fake node에 요청한 operation은 fake node의 inputQueue에 들어가나요 ?
• fake node로의 connect 요청 실패로 fake node는 reconnectQueue에 들어갈 것 같은 데, 맞나요 ?
• fake node의 inputQueue에 들어가 operation에 대해 timeout이 발생 후, 그 operation을 inputQueue에서 제거하는 것은 언제 하나요 ?
  • arcus client의 shutdown 요청 시에 하나요 ?

일단, review는 완료했고요.. 3개의 commit으로 구성되어 있는 데, 의미상 하나의 commit으로 merge하는 것이 좋을 것 같아요..

[aiceru]

@jhpark816 명시적으로 operation 을 inputQueue 에서 제거하는 부분은 없지만, 말씀하신 대로 MemcachedClient, MemcachedConnection, MemcachedNode 의 shutdown 과정을 거치면서 큐 자체가 소멸하게 됩니다. Selector 와 SocketChannel 도 모두 close 처리되구요.

commit 은, 이전에 다른 브랜치에서 CI test pass 를 위하여 이 브랜치의 내용을 merge 한 이력이 있어 rebase 시 혼란이 예상됩니다. 커밋 로그를 헷갈리지 않도록 신경써서 정리해 두었으니 이대로 merge 해도 무리 없지 않을까 싶습니다 ^^;;

• 원칙적으로... squash & rebase 는 개발자가 local 에서 commit 을 마음대로 사용하고, remote repository 에 올리기 전에 history line 이 너무 지저분해지지 않도록 관리하기 위한 과정입니다. 이미 remote repository 에 push 하여 다른 개발자가 pull or merge 하였을 가능성이 있는 branch 를 rebase, force push 하는 것은 대부분의 오픈소스 프로젝트에서도 금지하고 있는 사항입니다.

[whchoi83]

@aiceru merge 할 때 (conflict 가 아닌 이상) 최신 branch 의 내용이 반영되어야 할 필요가 없네요. 이것도 착각했네요.

[jhpark816]

commit 관련하여,

• 현재는 wooseok/fix_testTimeout 라는 개인의 작업 브랜치에 commit된 사항이라, 다른 개발자가 pull or merge하였을 리가 없습니다.. 혹, 우석님이 wooseok/fix_testTimeout 기반으로 다른 작업 브랜치를 만든 건가요 ?
• 현재의 3개 commit으로도 의미상 크게 문제는 없다고는 보나, 가능하다면 1개 commit이 더 좋지 않나 생각합니다.

[aiceru]

@jhpark816 이전에 민우씨의 map collection 기능추가 브랜치에서 CI test pass 를 위해 merge 한 이력이 있습니다 ^^;;;

[jhpark816]

@aiceru 그렇다면, 현재 상태로 merge 하세요..

[aiceru]

Fix issue #5