Skip to content

Comments

FIX: Invalid opType in XopGet operation instance.#884

Merged
jhpark816 merged 1 commit intonaver:developfrom
brido4125:fix/xopGet
Feb 28, 2025
Merged

FIX: Invalid opType in XopGet operation instance.#884
jhpark816 merged 1 commit intonaver:developfrom
brido4125:fix/xopGet

Conversation

@brido4125
Copy link
Contributor

@brido4125 brido4125 commented Feb 27, 2025

🔗 Related Issue

enum 타입의 필드가 불변으로 바뀌지 않아서
부정확한 read / write OperatoinType이 발생하는 문제
(아래 코멘트 참고)
#879 (comment)

⌨️ What I did

다양한 구현들을 생각해보았는데, 앞서 언급된 의견들 전부 고려해서
현재 PR이 제일 깔끔하다고 생각들었습니다.

XopGet의 경우 기본적으로는 read opType을 가지고,
op 생성자 내에서 delete 옵션이 true인 경우
setter를 통해 op 타입을 write로 변경합니다.

기존 findNodeByKey의 경우는 APIType을 인자로 받고 해당 인자내의 OpType을 사용했었는데요,
이 대신 Op 자체를 넘겨서 setter를 통해 변경된 Op를 사용하도록 변경했습니다.

groupingKeys와 같은 Op 인스턴스가 생기기전에 노드를 찾아야 하는 경우에만
기존의 APIType을 인자로 받는 findNodeByKey를 호출하도록 변경했습니다.

Copy link
Collaborator

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 완료


private boolean redirectSingleKeyOperation(String key, Operation op) {
return redirectSingleKeyOperation(findNodeByKey(key, op.getAPIType()), op);
return redirectSingleKeyOperation(findNodeByKey(key, op), op);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MemcachedClient.java 파일에 있는 groupingKeys()와 같이
operation 객체를 아직 생성하지 않은 경우에서 key들의 분배 시에는 어떻게 처리할 계획인가요?

Copy link
Contributor Author

@brido4125 brido4125 Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key들의 분배 시에는 어떻게 처리할 계획인가요?

질문이 잘 이해가 안가는데, 아래 답변이 아닌것 같으면 말씀해주세요
groupingKeys에서는 findNodeByKey(key, ApiType)을 그대로 호출합니다.
현재 PR에서는 오버로딩을 통해 op 객체를 인자로하는 메서드를 추가시킨것입니다.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. 이 부분은 변경이 없어도 되는 지를 확인해 볼게요.

@jhpark816
Copy link
Collaborator

@oliviarla 리뷰 바랍니다.

@jhpark816 jhpark816 requested a review from uhm0311 February 28, 2025 03:25
@jhpark816
Copy link
Collaborator

@brido4125
PR의 수정 내용을 정리하면, 아래와 같네요.

  • ApiType에 내포된 OpType은 그대로 유지
  • Operation 객체 생성 전에 key 분배할 시에는 ApiType 이용
    • 이 경우의 ApiType은 잘못된 opType을 가지는 경우가 없으므로 문제 없음
    • 만약, Collection Get 같은 연산에 대해 op 생성 전에 요청을 보낼 node를 찾아야 한다면, 문제가 될 수 있음
  • 그 외의 경우는 Operation 기준으로 요청을 보낼 node를 선택
    • Operation 객체에는 ApiType 외에 정확한 operation Type을 따로 유지하고 있으며,
    • 요청을 보낼 node 선택 시에 ApiType에 내포된 opType을 사용하지 않고,
      Operation 객체에 있는 정확한 operation Type을 사용함.

일단 repo에 있는 버그를 해겷하기 위해, 현 상태의 PR을 merge 생각입니다.
Operation 객체 생선 전에 key 분배할 시에 ApiType 이용하는 것은
다른 좋은 방안이 있는 지를 한번 더 검토해 볼 필요가 있습니다.

@jhpark816 jhpark816 merged commit f51c944 into naver:develop Feb 28, 2025
2 checks passed
@jhpark816
Copy link
Collaborator

@oliviarla @uhm0311
본 PR이 merge 되었지만, 수정 부분을 확인하고 문제 될 만한 부분이 있다면 알려주세요.

return locator.getPrimary(key);
}

public MemcachedNode getPrimaryNode(final String key, final Operation op) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public이지 않아도 되는 메서드들은 private / package private으로 변경해주세요.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants