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

FIX: getOperationStatus method in asyncCollectionPipedInsert/Update. #621

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

brido4125
Copy link
Collaborator

#619
로부터 파생된 PR입니다.
piped 연산에서 getOperationStatus의 로직을 수정했습니다.

@brido4125 brido4125 requested a review from jhpark816 June 15, 2023 05:40
@brido4125 brido4125 self-assigned this Jun 15, 2023
}
}
return new CollectionOperationStatus(true, "END", CollectionResponse.END);
return mergedOperationStatus.get(0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

receivedStatus를 호출하는 곳에서 성공한 op의 status는
아래와 같이 설정하여 인수로 넣어줍니다.

기존 로직인 new CollectionOperationStatus(true, "END", CollectionResponse.END)와 동일합니다.

public class CollectionPipedUpdateOperationImpl extends OperationImpl implements
        CollectionPipedUpdateOperation {
private static final OperationStatus END = new CollectionOperationStatus(
          true, "END", CollectionResponse.END);
}

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.

리뷰 완료

}
}
return new CollectionOperationStatus(true, "END", CollectionResponse.END);
return mergedOperationStatus.get(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

mergedOperationStatus가 empty인 경우는 없지만,
향후의 empty 경우를 고려하여 이 부분은 그대로 두시죠.

@brido4125
Copy link
Collaborator Author

mergedOperationStatus 필요 유무 검토

오프라인에서 논의한대로 mergedOperationStatus가 필요한 field값인지 검토해보았습니다.
아래는 서버로부터 발생하는 piepd 연산의 응답입니다.
한줄마다 command에 대한 상태가 담겨져 오는 형태입니다.

RESPONSE <count>\r\n
<STATUS of the 1st pipelined command>\r\n
<STATUS of the 2nd pipelined command>\r\n
...
<STATUS of the last pipelined command>\r\n
END|PIPE_ERROR <error_string>\r\n

예를들어 정상적으로 piped가 동작하면 아래와 같이 응답이 옵니다.
아래의 라인마다 담기는 STATUS 정보를 담아주는 field가
현재 로직에서의 mergedResult입니다.즉, 연산에 실패한 경우만 담습니다.
그리고 하나라도 실패한 경우에는
successAll이라는 boolean을 false로 설정합니다.

RESPONSE <count>\r\n
UPDATE \r\n
UPDATE \r\n
UPDATE \r\n

mergedOperationStatus의 상태는 위의 successAll이라는 field에 의해
값이 결정되므로 사실상 mergedResult에 의해 대체가 가능한 field입니다.
결론적으로 필요하지 않은 field라고 생각됩니다.

@brido4125 brido4125 marked this pull request as draft June 16, 2023 01:18
@brido4125 brido4125 marked this pull request as ready for review June 16, 2023 01:21
@brido4125
Copy link
Collaborator Author

brido4125 commented Jun 21, 2023

mergedOperationStatus 필요 유무 재검토

위 코멘트의 경우 해당 연산이 application단에서 cancel()이 호출되는 경우를 제외하고 작성되었습니다.

만약 리턴된 Future를 통해 op의 cancel()이 호출될 경우,
존재하는 복수개의 op들마다 아래의 op.cancel()이 호출됩니다.

//BaseOperatinImple
public final void cancel(String cause) {
    cancelled = true;
    if (handlingNode != null) {
      cancelCause = "Cancelled (" + cause + " : (" + handlingNode.getNodeName() + ")" + ")";
    } else {
      cancelCause = "Cancelled (" + cause + ")";
    }
    wasCancelled();
    callback.complete();
  }

해당 메서드 로직에서는 wasCancelled()를 호출하고 이는 아래와 같습니다.

  protected void wasCancelled() {
    getCallback().receivedStatus(STORE_CANCELED);
  }

즉, 취소된 op의 상태를 receivedStatus를
통해 mergedOperationStatus에 담아주고 있습니다.

그래서 mergedOperationStatus 사용하지 않도록 메서드 내부에서
receivedStatus가 아닌 gotStatus()를 호출하도록 변경시도 했지만,
piped연산의 gotStatus는 연산의 index 정보를 인수로 받기 때문에
불필요한 로직 변경이 생깁니다.

결론적으로 mergedOperationStatus라는 field값은 유지되어야합니다.

@jhpark816
Copy link
Collaborator

jhpark816 commented Jun 21, 2023

@brido4125
CI test 통과시키기 위해, rebase 부탁합니다.
다른 PR들도 마찬가지로 rebase해야 할 것 같습니다.

@jhpark816 jhpark816 merged commit ff43813 into naver:develop Jun 21, 2023
@brido4125 brido4125 deleted the fix/pipedOpereationStatus branch June 21, 2023 05: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.

2 participants