ENHANCE: Change asyncSetPipedExist method logic. #628
Conversation
brido4125
force-pushed
the
enhance/asyncSetExist
branch
2 times, most recently
from
0928e2f to
dd35dd9
Compare
brido4125
force-pushed
the
enhance/asyncSetExist
branch
from
dd35dd9 to
9a0d84c
Compare
|
@uhm0311 리뷰 바랍니다. |
|
@uhm0311 |
|
@jhpark816 |
brido4125
force-pushed
the
enhance/asyncSetExist
branch
3 times, most recently
from
c4df236 to
f6a5904
Compare
| getLogger().warn("Unhandled state: " + status); | ||
| cstatus = new CollectionOperationStatus(status); | ||
| } | ||
| rv.addOperationStatus(cstatus); |
There was a problem hiding this comment.
gotStatus()와 receivedStatus() 모두에서 rv.addOperationStatus() 호출하는 부분이 이상한 데,
문제가 없는 지 검토 바랍니다.
There was a problem hiding this comment.
gotStatus()와 receivedStatus()에서 rv.addOperationStatus()를 호출하면
응용이 getOperationStatus를 통해 단순히 FAILED_END 응답을 받는것이 아닌
UNREADABLE, TYPE_MISMATCH, NOT_FOUND 등 구체적인
실패의 원인에 대한 응답을 받을 수 있습니다.
로직 순서 상, gotStatus가 먼저 호출되기에 구체적으로 실패한 원인이
mergedOperationStatus 리스트에 먼저 담기고
최종적으로 receivedStatus를 통해 FAILED_END라는 응답을 추가합니다.
다른 종류의 Piped 연산인
asyncCollectionPipedInsert과 asyncCollectionPipedUpdate는
gotStatus에서 모든 응답값을 결과에 put 해주기 때문에
위와 같은 구현이 아니더라도 사용자가 구체적인 응답값을 확인할 수 있습니다.
결론적으로 사용자가 구체적인 실패 원인을 확인할 수 있다는 점에서
현재 구현을 유지해야한다고 생각합니다.
brido4125
force-pushed
the
enhance/asyncSetExist
branch
from
f6a5904 to
7c39158
Compare
|
@namsic |
brido4125
force-pushed
the
enhance/asyncSetExist
branch
2 times, most recently
from
80a8709 to
576c51e
Compare
brido4125
force-pushed
the
enhance/asyncSetExist
branch
from
576c51e to
6dc35da
Compare
brido4125
force-pushed
the
enhance/asyncSetExist
branch
2 times, most recently
from
9209c10 to
febf089
Compare
| getLogger().warn("Unhandled state: " + status); | ||
| } | ||
| if (failedStatus != null && isSameStatus) { | ||
| rv.addOperationStatus(failedStatus); |
There was a problem hiding this comment.
- 해당 operation으로 EXIST or NOT_EXIST 응답이 하나라도 있다면, FAILED_END로 설정해야 합니다.
- migration인 경우 고려
- 하나의 operation으로 2개 operation으로 나누어 처리될 수 있습니다.
- 2개 중 하나는 END이고 다른 하나는 FAILED_END이면, 최종 결과는 FAILED_END 이어야 합니다.
There was a problem hiding this comment.
2개 중 하나는 END이고 다른 하나는 FAILED_END이면, 최종 결과는 FAILED_END 이어야 합니다.
현재 로직도 사용자가 getOperationStatus를 통해
받게되는 최종 결과는 FAILED_END입니다.
brido4125
force-pushed
the
enhance/asyncSetExist
branch
from
febf089 to
041464e
Compare
|
@brido4125 rebase 한번 해 주세요. |
brido4125
force-pushed
the
enhance/asyncSetExist
branch
from
041464e to
21d9645
Compare
brido4125
force-pushed
the
enhance/asyncSetExist
branch
from
21d9645 to
166686f
Compare
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
아래 경우에 setOperationStatus 부분이 문제가 될 것 같습니다.
- 2개 연산으로 pipeling 처리
- 처음 연산에서는 모두 EXIST or NOT_EXIST 응답이 옴.
- 둘째 연산에서는 NOT_FOUND 응답이 옴.
이 경우, 최종 OperationStatus는 NOT_FOUND가 됩니다.
맞는 동작인 지 검토 바랍니다.
There was a problem hiding this comment.
#628 (comment)
위 코멘트대로, 여러 Op 중 하나라도 FAILED_END 라면,
Future에서 반환하는 응답값은 FAILED_END로 처리하는게 맞지 않나요?
반대의 상황에서도 FAILED_END는 유지가 됩니다.
다만, PR 내의 failStatusCount는 삭제해도 될 것 같습니다.
There was a problem hiding this comment.
바로 위의 코멘트 (https://github.com/naver/arcus-java-client/pull/628/files#r1530238965) 에 보인 예시에서는 FAILED_END가 아닌 NOT_FOUND가 최종 OperationStatus로 되지 않나요?
There was a problem hiding this comment.
맞는 동작인 지 검토 바랍니다.
최종 OperationStatus가 실패 / 성공의 여부에 대해 맞는 동작인지 검토하는것으로 이해했습니다.
말씀하신 부분은 수정되어야 합니다.
@jhpark816
Docs를 확인해보니 단순히 FAILED_END가 아닌 구체적인 이유가 들어간 OperationStatus를 반환합니다.
https://github.com/naver/arcus-java-client/blob/develop/docs/05-set-API.md#set-element-%EC%9D%BC%EA%B4%84-%EC%A1%B4%EC%9E%AC%EC%97%AC%EB%B6%80-%ED%99%95%EC%9D%B8
기존 동작으로 수행해야 하위 호환성 문제가 없을 것 같네요.
There was a problem hiding this comment.
현재 구현은 count를 통해서 해당 pipe Op의 모든 연산들이 동일한 원인인 경우,
구체적인 NOT_FOUND, TYPE_MISMATCH, UNREADABLE 을 리턴합니다.
만약 연산 중 일부는 성공 / 일부는 실패일 경우는 FAILED_END를 리턴하네요.
하지만 FAILED_END라는 의미가 디버깅에도 별 도움이 되지 않기 때문에,
하나라도 NOT_FOUND, TYPE_MISMATCH, UNREADABLE 중 설정 된다면
해당하는 Status를 리턴시켜 주는게 도움이 될 것 같습니다.
There was a problem hiding this comment.
@brido4125
코멘트에 있는 대로 처리하면 안 될 것 같습니다.
N개 pipe 연산에서 i번째 연산이 실패하면 그 뒤에 있는 모든 연산도 함께 실패 처리하는 것이 나을 것 같습니다.
이렇게 구현하는 것이 가능할까요?
@uhm0311 @oliviarla
위의 사항 검토 바랍니다.
위의 사항 때문에 기존에 piped 연산은 1개 연산으로만 제공했던 것은 아닌지 ?
There was a problem hiding this comment.
N개 pipe 연산에서 i번째 연산이 실패하면 그 뒤에 있는 모든 연산도 함께 실패 처리하는 것이 나을 것 같습니다.
이렇게 구현하는 것이 가능할까요?
그러면 N개 pipe 연산을 보낼 때 1개를 보내고 응답을 받아서 실패하지 않았으면 다시 다음걸 보내는 방식으로 해야 합니다.
JDK 8에서는 CompletableFuture가 있으므로 1개 pipe 연산에 대한 CompletableFuture의 성공 Callback에 다음 pipe 연산을 보내는 식으로 구성하여 실패 시 다음 연산을 보내지 않도록 할 수 있습니다.
JDK 6에서는 ListenableFuture를 구현하지 않는 이상 비동기 방식은 불가능할 것으로 보입니다.
단, N개의 연산을 보낼 때 1개를 보내고 응답을 받아서 다음걸 보내는 방식은 N개 연산을 다 처리하는 데에는 지연 시간이 더 걸리게 되어 있으므로 Timeout 에러가 발생할 확률이 높아집니다.
There was a problem hiding this comment.
그러면 N개 pipe 연산을 보낼 때 1개를 보내고 응답을 받아서 실패하지 않았으면 다시 다음걸 보내는 방식으로 해야 합니다.
N개 pipe 연산을 보낼 때, 중간에 에러 발생 등으로 pipeline이 중단되는 경우가 아니면 마지막 N번째 연산을 보내기 전까지는 응답이 돌아오지 않을텐데요.
1개를 보내고 응답을 받은 뒤 다음 pipe 연산을 보낸다는 것의 의미가 제가 이해한 것과 다른 동작일까요?
There was a problem hiding this comment.
500개의 연산을 1개의 pipe 연산으로 지칭한 것입니다.
There was a problem hiding this comment.
위의 사항 때문에 기존에 piped 연산은 1개 연산으로만 제공했던 것은 아닌지 ?
exists 파이프만 1개 연산 지원이고, 다른 pipe는 초기 커밋부터 n개를 허용하였기에 그렇다고 보긴 힘들것 같네요
uhm0311
force-pushed
the
develop
branch
2 times, most recently
from
07918e6 to
ae2a82f
Compare
관련 이슈
Motivation
기존 asyncSetPipedExist는 하나의 Operation 인스턴스만 지원한다.
이를 다른 Pipe 연산과 동일하게 Elem의 개수가 500개가 넘어가도
정상적인 api 처리를 해주는 로직으로 변경한다.
변경 지점
기존에 존재하는 asyncSetPipedExist의 연산 로직을 변경하였습니다.
우선 500개의 요소 단위로 하나의 연산을 구성하여
multi operation 인스턴스에 대한 지원이 가능하도록 변경하였습니다.
기존 로직과 달리 여러개의 op연산이 발생하기 때문에
다른 piped api들과 마찬가지로
mergedOperationStatus을 사용합니다.