move queueReconnect line because of switchover

whchoi83 commented 8 years ago

https://github.com/jam2in/arcus-java-client/issues/11 이슈를 반영한 PR 입니다.

updateConnection 에 의한 switchover 시 전체 operation 을 move 할 수 있도록 queueReconnect 호출 위치를 변경하였습니다. (MemcachedConnection.switchoverMemcachedReplGroup 참고) 추가로 slave node 의 operation 을 move 할 때 setupResend 를 수행하지 않도록 했습니다. (slave node 은 read operation 만 가능하기 때문에 전체 operation 을 move 해도 무방합니다.)

추가로 기존 MoveOperationTask 는 Locator.update() 후에 하도록 되어 있는데, 이것을 after task 로 통일 setupResend 와 queueReconnect 는 before task 로 정의하여 수행하도록 했습니다.

리뷰 부탁드립니다.

[x] @aiceru
[x] @jhpark816

whchoi83 commented 8 years ago

task list 를 한 곳으로 모아도 상관없을 것 같습니다. 해당 코드로 수정 후에 다시 리뷰 부탁드립니다.

whchoi83 commented 8 years ago

하나의 list 에서 locator update 후에 setupResend/QueueReconnect 와 move Operation 을 수행해도 문제가 없다고 판단되어 task list 를 하나로 합쳐서 코드 commit 했습니다.

리뷰 부탁드립니다. @aiceru @jhpark816

aiceru commented 8 years ago

review 완료하였습니다. 특이사항 없어 보입니다.

jhpark816 commented 8 years ago

switchover 시에 cache_list의 변경은 아래 순서로 발생합니다..

처음 상태가 아래와 같다고 가정합니다. 11215가 master 입니다.

Sep  5 11:36:10 jam2in-t002 memcached[12046]: server[0] = g0^M^127.0.0.1:11215-localhost
Sep  5 11:36:10 jam2in-t002 memcached[12046]: server[1] = g0^S^127.0.0.1:11216-localhost

master switchover 요청하여, switchover 완료 시점에 zoo_multi()로 자신의 znode를 변경합니다. 즉, 어느 한 시점에 두 znode가 slave 상태로 설정됩니다.

Sep  5 11:36:40 jam2in-t002 memcached[12046]: server[0] = g0^S^127.0.0.1:11216-localhost
Sep  5 11:36:40 jam2in-t002 memcached[12046]: server[1] = g0^S^127.0.0.1:11215-localhost

그리고 나서, slave가 switchover 작업을 완료하면, zoo_multi()로 자신의 znode를 변경합니다. 결국, 11216이 master가 되게 됩니다..

Sep  5 11:36:40 jam2in-t002 memcached[12046]: server[0] = g0^M^127.0.0.1:11216-localhost
Sep  5 11:36:40 jam2in-t002 memcached[12046]: server[1] = g0^S^127.0.0.1:11215-localhost

즉, 한번에 두 개의 znodes가 변경되는 것이 아니라, 하나씩 차례로 변경되게 됩니다. 이와 같이 중간 변경 상황에서 ZK event를 받을 때, client는 정상 동작하는 가요 ?

참고로, 위의 예는 실졔 switchover 상황에서 cache server가 기록했던 실제 메세지입니다.

jhpark816 commented 8 years ago

질문이 하나 있는 데요.. 아래 코드에서 newGroupAddrs = null이면, 메세지 출력이 정상적으로 되나요 ?

ArcusClient.arcusLogger.debug("New group nodes : " + newGroupAddrs);

jhpark816 commented 8 years ago

위에서 질문했던 slave + slave 구성된 replica group은 invalid group으로 분류되어 hash ring에서 그 replica group은 변경하지 않네요...

jhpark816 commented 8 years ago

Switchover case 시에, move operations 하고 나서, old master에 대해 queue reconnect를 할 필요가 있나요 ??

whchoi83 commented 8 years ago

@jhpark816 slave / slave 구성에 대해서는 updateConnections 에서 makeGroupAddrsList 를 호출하여 비정상적인 group 상태는 반영하지 않도록 논의하여 적용했던 사항입니다.

old master 를 queueReconnect 하는 이유는 (만일의 경우에 대비하여 찌거기) operation 을 제거해야 한다고 하셔서 우선 반영했던 것이고, 정확히 필요한지는 [Server] worker thread 종료 시의 request atomicity 보장 확인 이슈 점검 등을 통해서 점검 후 결정하기로 했습니다. 현재까지는 close -> connect 과정이 필요하지 않다는 100% 보장이 없기 때문에 확인 후 적용하도록 여러 차례 논의했습니다. 서버 코드 점검 후 적용 여부를 결정하도록 하겠습니다. 혹시라도 중간에 확인이 되면 본 PR의 issue 에 코멘트 부탁드립니다.

jhpark816 commented 8 years ago

@whchoi83 이미 전체 operations을 move 시킨 상태라서, 추가로 제거해야 할 operations이 없지 않나요 ? switchover 경우라서, worker thread 종료 처리와도 관련성이 없을 것 같고요..

whchoi83 commented 8 years ago

old master 를 queueReconnect 하는 이유는 (만일의 경우에 대비하여 찌거기) operation 을 제거해야 한다고 하셔서 우선 반영했던 것이고,

이 부분은 클라이언트 쪽 이야기가 아니라 서버 쪽 이야기입니다. client 쪽은 move operation 을 통해 operation 자체 처리는 모두 완료된 상태입니다. 서버 쪽에도 100% 보장이 된다면 연결을 재설정하는 부분은 없어도 될 것 같습니다.

jhpark816 commented 8 years ago

@whchoi83 queueReconnect 처리는 필요하네요..

cache server의 worker thread 종료 처리와는 무관하고,
Client 쪽 IO queue에서 operations을 제거하는 용도는 아니며,
예전 reading state에 있던 operations에 대해 cache server로 부터 어떤 형태로든 response가 올 수 있기 때문이네요.

whchoi83 commented 8 years ago

@jhpark816 본 PR의 Issue 내용 중 https://github.com/jam2in/arcus-java-client/issues/11#issuecomment-239341628 후에 오프라인으로 아래 내용에 대해서 다음과 같은 질문을 드렸던 적이 있습니다.

예전 reading state에 있던 operations에 대해 cache server로 부터 어떤 형태로든 response가 올 수 있기 때문이네요.

예를 들어 Client 가 5개의 operation (1. W op, 2 ~ 5. R op) 을 server 에게 request 했을 때 server 가 1번째 Write operation 에 대해서 switchover reponse 를 client 에게 보낸다면 그 이후의 2~5번째 operation 들은 어떻게 처리하는지 궁금합니다. 이전에 대답해주셨을 때 (정확한 표현은 기억나지 않지만) 처리하지 않는다? 버린다? 라는 형태의 대답을 하셨던 걸로 기억합니다.

코멘트 작성하는 중 생각난 문제인데, switchover 가 완료되고, ZK로 부터 event 를 받기 전에 client 가 새로운 slave (old master)로 write request 를 보내는 경우, REPL_SLAVE를 받으면 move operation 이 수행되는데, 이때 보냈던 operation 의 response 들은 받을 수 있기 때문에 queueReconnect 는 여전히 필요 할 것 같습니다.

결론은 필요하다가 되겠네요.

jhpark816 commented 8 years ago

setupResend/QueueReconnect를 locator update 후에 수행하도록 변경되었는 데, setupResend/QueueReconnect 외에 MoveOperations을 locator update 이전에 수행하면 문제가 될까요 ?

whchoi83 commented 8 years ago

@jhpark816 초기 개발 시엔 locator update 이전에 수행하도록 했다가 locator.update 를 빨리 수행해 주는 것이 좋을 것 같다고 조언하셔서 논의 후 결정한 사항입니다.

예를 들어 move operation 을 먼저 수행하게 되면. move operation 작업 이후, locator update 수행 이전에 API를 통해 operation 이 요청된다면, remove 될 node 에 operation 을 요청하는 결과가 발생할 수도 있습니다.

jhpark816 commented 8 years ago

@whchoi83 OK. Thank you !!

locator update를 빨리하는 것이 유리하네요..
그 이후에 move operations을 처리하는 것이 정확성에도 맞네요..
- move operations 이후에 remove할 node에 new operation이 들어가지 않도록 하기 위해..

jhpark816 commented 8 years ago

@whchoi83 아래 코드에서 newGroupAddrs = null이면, 메세지 출력이 정상적으로 되나요 ?

ArcusClient.arcusLogger.debug("New group nodes : " + newGroupAddrs);

whchoi83 commented 8 years ago

@jhpark816 newGroupAddrs = null 인 경우 "null" 로 자동 변환됩니다. 즉, New group nodes : null 형태로 출력됩니다.

jhpark816 commented 8 years ago

@whchoi83 @aiceru 코드 리뷰 완료... 의견입니다.

현재 commit 된 내용은 기본보다 개선된 코드이다.
- switchover 시에 모든 연산 이동
- slave node에 있는 연산들은 read only 연산이므로, 모두 move 시키자.
- locator update 이후에, moveOperations/setupResend/queueReconnect 처리하는 것이
- 응용에게 정확한 hash ring을 보다 빨리 제공하고,
- moveOperations은 locator update 이후에 해야 그 정확성이 보장되고, (빠지는 operation이 없음)
- move해야 할 operations 수가 조금이라도 적어진다.
추가 검토/개선해야 할 부분은 새로운 이슈로 등록하여 처리하자.

다른 이견 없으면 merge 할께요..

jhpark816 commented 8 years ago

아. 여기도 beforetask와 aftertask 등의 중간 코드가 들어가 있는 데... 1개 commit이면 좋을 것 같긴 합니다..

whchoi83 commented 8 years ago

1 commit 으로 수정했습니다.

jam2in / arcus-java-client

move queueReconnect line because of switchover #17