Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

INTERNAL: Change failedOpreationStatus and resultOperationStatus from List<OperationStatus> to CollectionOperationStatus. #705

Merged
merged 1 commit into from
Dec 26, 2023

Conversation

uhm0311
Copy link
Collaborator

@uhm0311 uhm0311 commented Dec 21, 2023

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

위 이슈 일부를 반영했습니다.

  • OperationStatus List 타입 제거

Copy link
Collaborator

@brido4125 brido4125 left a comment

Choose a reason for hiding this comment

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

receivedStatus() 메서드구현에 해당 연산은 multi operation이지만 다른 api들과 달리
List<OperationStatus>를 가지지 않는다는 주석이 추가되면 좋을것 같습니다.

@uhm0311
Copy link
Collaborator Author

uhm0311 commented Dec 22, 2023

@brido4125

주석 추가했습니다.

}
return new CollectionOperationStatus(resultOperationStatus.get(0));
return resultOperationStatus;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@uhm0311
질문입니다.

아래 경우에서 getOperationStatus() 호출되는 경우는 없는 거죠?
즉, smget 연산이 merge 작업이 완료되어 failed/result 중의 하나가 설정된 경우만 getOperationStatus() 호출될 수 있는 거죠?

  • 기존 방식에서 failed/result OperationStatus가 모두 empty인 경우
  • 신규 방식에서 failed/result OperationStatus가 모두 null인 경우

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OperationStatus가 설정된 뒤에 OperationCallback.complete()가 호출되어 Future.get()의 블로킹이 해제되므로 그럴 일은 없을 것 같습니다.

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.

리뷰 완료

@@ -109,6 +112,10 @@ public void mergeSMGetElements(final List<SMGetElement<T>> eachResult,

@Override
public void makeResultOperationStatus() {
if (resultOperationStatus != null) {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

위와 같이 null 아닌 경우는 리턴하는 코드를 넣을 필요가 있나요?

@@ -2108,7 +2108,9 @@ public void receivedStatus(OperationStatus status) {
}
} else {
stopCollect.set(true);
result.addFailedOperationStatus(status);

// Multi key operation but SMGetFuture returns only one, the first failedOperationStatus.
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기 주석은 없어도 될 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#705 (review)

위 의견에 따라 추가한 주석인데, 그래도 제거할까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

예 제거하시죠.
다른 곳의 OperationStatus 처리도 변경될 것으로 예상됩니다.

… List<OperationStatus> to CollectionOperationStatus.
@uhm0311
Copy link
Collaborator Author

uhm0311 commented Dec 26, 2023

@jhpark816

모두 반영했습니다.

@jhpark816 jhpark816 merged commit 967c9ef into naver:develop Dec 26, 2023
1 check passed
@uhm0311 uhm0311 deleted the uhm0311/develop4 branch December 27, 2023 08:10
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