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: getElement logic in BTreeStoreAndGetFuture. #662

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

brido4125
Copy link
Collaborator

Motivation

기존 BTreeStoreAndGetFuture의 getElement 동작은
곧바로 element 변수를 리턴하는 로직인데
이는 Future의 get()이 완료되지 않을 경우
리턴되는 결과가 부정확하다.

즉, 응용의 Worker Thread가 Blocking되어 정확한 결과값을 리턴받도록 변경해야한다.

getElement() 내부적으로 get()을 호출하여
정확한 element를 리턴하도록 변경한다.

@brido4125 brido4125 self-assigned this Sep 1, 2023
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.

리뷰 완료

throw new RuntimeException(e);
} catch (ExecutionException e) {
throw new RuntimeException(e);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 코드를 가장 상위에 두면 좋겠습니다.
OperationTimeoutException 을 던저야 하는 지도 검토 바랍니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

호출되는 get() 메서드 내에서
CheckedOperationTimeoutException를 던지고
이를 TimeOutException에서 catch 하여
RuntimeException을 생성하기에
새롭게 생성하여 Throw할 필요는 없다고 생각합니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

try 문이 가장 앞애 나오개 합시다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@uhm0311 exception 부분 리뷰해 주세요.

참고로, OperationFuture.get() 코드에서도
명시적인 OperationTimeoutException 던지는 것 같습니다.

  public T get() throws InterruptedException, ExecutionException {
    try {
      return get(timeout, TimeUnit.MILLISECONDS);
    } catch (TimeoutException e) {
      throw new OperationTimeoutException(e);
    }
  }

Copy link
Collaborator

Choose a reason for hiding this comment

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

@brido4125
TimeoutException에 한해서 다음과 같이 합시다.

...
} catch (TimeoutException e) {
  throw new OperationTimeoutException(e);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@brido4125
나도 같은 의견이니, 이렇게 수정해 주시죠.

@brido4125 brido4125 force-pushed the fix/BTreeStoreAndGetFuture branch 2 times, most recently from 04ffb09 to 19b8f23 Compare September 5, 2023 23:39
@jhpark816 jhpark816 merged commit 5a3d4db into naver:develop Sep 11, 2023
1 check passed
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