Refactoring: Fix unit tests

[aiceru]

3 #4 관련 수정 PR 입니다. (20일 수요일 오전에 논의 제안했던 내용입니다. 해결책이 보여 우선 PR 작성합니다. 논의하기로 했던 시간에 코드 내용 및 배경 설명하고 간략히 함께 리뷰하는 시간으로 활용하면 될 것 같습니다.)

• USE_ZK 옵션은 true / false 두 경우 모두 수행하는 것으로 살려 두었습니다.
• CFB와 inner object 가 올바른 ConnectionObserver 를 참조할 수 있도록 수정하였습니다. CFB 자체를 완전히 걷어내는 수정은, spymemcached 와 완전 결별-_-을 선언하지 않는다면 불가능합니다.
• Maven test 수행시 test failure 또는 error 가 발생하면 즉시 남은 test 를 skip 하도록 mvn 설정을 수정하였습니다.
• 특정 테스트는 USE_ZK == true 일 때만 수행하도록 수정하였습니다. 이 과정에서 해당 테스트 클래스들을 JUnit 4 style 로 수정하였습니다. (기존의 구현은 JUnit 3 style)

Review

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

[jhpark816]

@aiceru review 완료..

[whchoi83]

(본 코멘트는 test 이기 때문에 무시해도 될 것 같긴한데 좀 애매하네요.)

현재 ClientBaseCase.initClient() 에서 anonymous DefaultConectionFactory 를 생성할 때 아래와 같이 FailureMode 를 retry 로 사용하고 있습니다.

            @Override
            public FailureMode getFailureMode() {
                return FailureMode.Retry;
            }
        });

USE_ZK = false 일 경우는, spymemcached 의 MemcachedClient 를 사용하고 모든 node 와의 연결이 connected 됐는지 확인하지 않은 상태에서 operation 을 요청하기 때문에 테스트 성공을 위해 retry 를 사용하여야 합니다.

하지만 USE_ZK = true 인 경우는, ArcusClient 에서 모든 node 와의 연결을 ConnectionObserver 를 통해서 확인을 하고 operation 을 요청하기 때문에 retry 를 사용하지 않아도 됩니다. 또한, 기본 권장 사항 역시 retry 가 아닌 cancel 이고요.

테스트 코드여서 retry 하여도 딱히 문제가 발생할 것 같진 않지만, 테스트 중이던 node 가 갑자기 죽거나, 네트워크에 문제가 생긴다거나 하는 등의 문제가 생기면 계속 retry 를 할 것 같습니다. 이런 경우라면 operation timeout 에 걸려 cancel 이 되거나 테스트가 종료되서 문제가 될 것 같진 않지만, USE_ZK 값에 따라서 retry 와 cancel 을 사용하도록 수정해야 하지 않을까 합니다.

[whchoi83]

리뷰 완료했습니다.

[aiceru]

pom.xml surefire 설정 주석처리, createConneciton 함수 코멘트, testInitialObservers USE_ZK false 일 경우에만 실행하도록 수정. (내부 코드도 직접 ConnectionFactoryBuilder 를 생성하여 사용하도록 수정했습니다.)

FailureMode 수정에 관해선 저도 @whchoi83 과 동일한 의견입니다. @jhpark816 의견 부탁드립니다~

[whchoi83]

ObserverTest 수정 코드를 보다가 문득 생각난 것인데, 어차피 openDirect 는 CFB 를 사용하지 않아도 되는데

    protected void initClient(ConnectionFactory cf) throws Exception {
        if (USE_ZK) {
            openFromZK(new CFB(cf));
        } else {
            openDirect(cf);
        }
    }

    protected void openFromZK(ConnectionFactoryBuilder cfb) {
        client = ArcusClient.createArcusClient(ZK_HOST, ZK_SERVICE_ID, cfb);
    }

    protected void openDirect(ConnectionFactory cf) throws Exception {
        client = new ArcusClient(cf, AddrUtil.getAddresses(ARCUS_HOST));
    }

이와 같이 수정하는 것이 나을 것 같습니다. 어떤가요? ObserverTest 에도 assumTrue 코드만 넣을 수 있고, 기존 (ArcusClient 를 포함해) spymemcached 코드의 의도와도 비슷하게 테스트 될 것 같습니다.

[jhpark816]

위와 같이 고칠 바에야, 아래와 같이 고치는 것이..

    protected void initClient(ConnectionFactory cf) throws Exception {
        if (USE_ZK) {
                client = ArcusClient.createArcusClient(ZK_HOST, ZK_SERVICE_ID, new CFB(cf));
        } else {
               client = new ArcusClient(cf, AddrUtil.getAddresses(ARCUS_HOST));
        }
    }

[whchoi83]

@jhpark816 아 그렇네요. 😅 별 생각없이 전체 code 붙여놓고 삭제하면서 코드를 수정했더니 당연한 부분을 놓쳤네요.

[aiceru]

openDirect 의 인자만 ConnectionFactory 타입으로 바꾸고, 함수 통합은 하지 않겠습니다.

BaseIntegrationTest 라고... 이건 아마 모양을 보니 ARCUS 로 확장하면서 새로 만든것 같은데, ClientBaseCase 와 거의 같은 역할입니다. 여기에도 openFromZK, openDirect 가 있고... 이쪽의 openDirect 는 ArcusClient 생성시에 observer 를 작성하여 등록하고 latch.await 로 connect 를 기다립니다. 동급(?)의 클래스여서 가능한 같은 프로토타입을 유지하는 편이 코드 가독성에 좋다고 생각되기 때문입니다.

ClientBaseCase 와 BaseIntegrationTest 양쪽의 로직이 다른 건 보기가 좋지는 않지만, BaseIntegrationTest 의 로직 쪽이 더 깔끔하고 통일성있게 ArcusClient 를 사용하는 모양인 것 같습니다. 이번 수정사항은 기존 spymemcached 와 확장 ARCUS 의 구조적 차이에서 기인하는 것으로 (핵심은 ConnectionFactory 를 바로 사용 가능한가, builder 를 거쳐야만 하는가의 차이입니다) ClientBaseCase 쪽의 구현 (현재 수정된 모습) 을 일종의 예외허용 상황으로 보는 것이 좋을 것 같습니다.

더불어 FailureMode 의 경우, default cancel을 권장하고 있지만 timeout test 등 테스트 상황에서는 retry 가 필요한 상황이 많고 어떠한 목적을 가지고 특정동작을 테스트한다는 테스트코드의 특성상 일시적인 장애 등으로 cancel 이 바로 발생하는 것보다는, 최대한 원하는 동작이 발생할 수 있는 경우로 유도한다는 의미에서 retry 를 기본값으로 가져가는 편이 좋다고 판단됩니다.

[jhpark816]

@aiceru @whchoi83 리뷰 완료..

commit은 그대로 할 건가요 ? 아님 정리할 건가요 ?

[aiceru]

이 PR 은 conversation 을 그대로 살려두어야 추후 필요시 히스토리를 파악하기가 용이하다고 판단되기 때문에, commit 정리하지 않고 그대로 merge 하였습니다.