INTERNAL: Do not cancel operations when node is removed from ZK but still alive.

[uhm0311]

🔗 Related Issue

• https://github.com/jam2in/arcus-works/issues/535

AS-IS

• MemcachedNode와의 연결 해제 시 Operation Queue에 적재된 모든 Operation.cancel() 호출합니다.
• 위와 같은 동작은 연결 해제 도중에도 Operation을 성공시킬 가능성이 조금이나마 존재하지만 바로 캔슬시키는 문제가 있습니다.

TO-BE

• NodeLocator에서 노드를 제거할 때, 아래 2가지 조건을 모두 만족하면 op 캔슬을 하지 않고 DelayedClosingNodes로 분류합니다.
  • 아직 노드와의 소켓 연결이 해제되지 않았음
  • Operation Queue가 비어 있지 않음
• IO Thread가 DelayedClosingNodes로 분류된 노드들을 순회하며 다음의 로직을 수행합니다.
  • 소켓 연결이 끊어진 노드가 있다면 연산을 모두 캔슬하고 DelayedClosingNodes에서 제거합니다.
  • Operation Queue가 비어 있는 노드가 있다면 소켓 연결을 해제하고 DelayedClosingNodes에서 제거합니다.

[oliviarla]

handleNodesToRemove 메서드가 복제와 마이그레이션에서도 사용되는데, 해당 부분에는 영향이 없나요?

[uhm0311]

EE여도 cache_list znode가 제거되었을 때 동작은 CE와 동일합니다.
alter_list znode가 제거되었을 때는 NodeLocator에서 소켓 연결을 해제하므로 영향이 없습니다.

[uhm0311]

첫번째 코멘트에 커밋 수정사항을 업데이트 했습니다.

[jhpark816]

@oliviarla @brido4125 먼저 리뷰 하시죠.

[brido4125]

https://github.com/jam2in/arcus-works/issues/549

현재 PR에서는 locator의 update와 handleDelayedClosingNodes 모두 IO 쓰레드에서 순차적으로 수행합니다.

만약, 현재 진행중인 이슈처럼 locator에 대한 update 수행을 다른 스레드가 수행할 경우
해당 구현도 변경이 필요할 수도 있습니다.

추후 참고할 목적으로 해당 코멘트 남깁니다.

[brido4125]

delete 대상인 노드들을 Locator 단에서 구분하고
delayedClosingNodes 또한 Locator가 관리합니다.

근데 Locator의 클래스 의미를 생각한다면
한가지 책임을 추가하는게 좋은 설계는 아니라고 생각됩니다.

차라리 MemcachedConnection 쪽에서 update 또는 updateRepl을
호출 할 때 delete 대상이 되는 Collection을 넘기니
그쪽에서 처리하는건 어떤가요?

[uhm0311]

@brido4125

소켓 연결에 대한 해제를 수행하는 메소드 호출(node.closeChannel())이 대부분 NodeLocator 내에 있어서 그렇게 분리하기는 힘들 것 같습니다.
PR의 의도는 소켓 연결을 해제하기 전에 정말 해제할 필요가 있는지를 확인하는 것이므로 소켓 연결 해제 로직과 결합되어야 합니다.

[jhpark816]

@uhm0311 @oliviarla
본 PR은 아직 리뷰가 완료되지 않은 상태이며, 진행해야 할 작업입니다.

현 시점에서 보니, 아래의 보완이 필요합니다.

• 이슈에서 지적한 문제에 대한 AS-IS 동작과 PR에서 변경할 TO-BE 동작의 핵심만 정리하여 쉽게 설명 (@uhm0311 담당)
  • 다른 내용을 참고하라고 할 시에 링크를 넣어주더라도 그 내용의 요약은 여기 코멘트에 기재해야 합니다.
• 다시 리뷰 진행 (@oliviarla 담당)

[uhm0311]

@jhpark816

수정했습니다.

[jhpark816]

@uhm0311

이슈에서 지적한 문제에 대한 AS-IS 동작과 PR에서 변경할 TO-BE 동작의 핵심만 정리하여 쉽게 설명

위 내용은 어디에 정리되었나요?

[uhm0311]

@jhpark816

첫번째 코멘트에 있습니다.

[oliviarla]

@jhpark816 이 코드를 추가하면서 복잡함을 감수할 만큼 얻는 이득이 큰지 궁금합니다.

[oliviarla]

여기는 어떤 경우이고 왜 addedQueue에 다시 넣는지 궁금합니다.

[uhm0311]

연결이 아직 살아 있고, Op 큐에 연산이 남아 있습니다.
따라서 addedQueue에 넣어서 Op가 handleIO 로직에 의해 처리되도록 합니다.

[oliviarla]

hasOp여도 close해야 하지 않나요?
handleDelayedClosingNodes 메서드가 전체적으로 이해하기 어렵습니다.

[uhm0311]

연결이 살아 있고, Op 큐에 연산이 남아 있으면 계속 처리하려고 합니다.

[uhm0311]

@jhpark816

리뷰 의견 부탁드립니다.

[jhpark816]

@oliviarla

이 코드를 추가하면서 복잡함을 감수할 만큼 얻는 이득이 큰지 궁금합니다.

• TO-BE 설계가 적합한가요? 더 나은 방안은 없는 지 ?
• 기능 추가로 인한 긍정 효과와 복잡함이란 부정 효과가 공존합니다.
  둘 다를 고려할 때 AS-IS 동작이 나은가요?

@uhm0311
위의 동일한 질문에 대해 어떻게 생각하는 지 궁금합니다.

[uhm0311]

기능 추가를 통해 얻을 수 있는 이득이 미미하여 추가하지 않아도 될 것 같습니다.
더 나은 설계가 있는지는 모르겠습니다.

[oliviarla]

저도 TO-BE 설계보다는 AS-IS 동작이 나은 것 같습니다.

• 이러한 종류의 이슈가 자주 발생하지 않으므로, 코드를 복잡하게 만들만큼 중요한 사항이 아님
• 특히 캐시 서버의 소켓 연결은 남아있지만 znode가 사라지는 경우는 흔치 않음
• Operation을 대기하거나 redirect하는 것보다는 cancel시켜 사용자에게 상태를 알리는 것도 하나의 방법

[jhpark816]

@uhm0311 @oliviarla

• TO-BE 설계를 기반으로 하는 본 PR은 복잡성 대비 얻는 이득이 미미하여 반영하지 않겠습니다.
• 본 이슈에 대해 다른 설계 방안은 이슈에서 다시 다루겠습니다.
  https://github.com/jam2in/arcus-works/issues/535