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: add omitted parameter method - asyncBopGet() #522

Merged
merged 1 commit into from
Jul 22, 2022

Conversation

oliviarla
Copy link
Collaborator

@oliviarla oliviarla commented Jul 21, 2022

#439

  • withDelete, dropIfEmpty 인자만 생략한 asyngBopGet() API를 제공합니다.
  • byte array형식의 bkey를 사용하는 API들을 일관성 있게 위치 조정하였습니다.

@jhpark816
Copy link
Collaborator

@sUpniverse
API 코드 위치에 대해 검토해 주세요.

Copy link
Contributor

@sUpniverse sUpniverse left a comment

Choose a reason for hiding this comment

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

@oliviarla @jhpark816

리뷰 완료했습니다.

현재는 Collection 연산에 대해 overloading으로 구현된 method의 순서는
Transcoder를 제외한 모든 시그니처의 method를 선언한 다음에 Transcoder tc 시그니처를 추가한 순서로
구성이 되어 있습니다.

물론 중간에 Collection Insert() 메서드의 경우 같은 시그니처를 가진, method끼리 구성을 해두어서
이부분도 점검을 해보아야 할것 같네요

@@ -677,6 +693,13 @@ public CollectionFuture<Map<Long, Element<Object>>> asyncBopGet(String key,
count, withDelete, dropIfEmpty);
}

@Override
public <T> CollectionFuture<Map<Long, Element<T>>> asyncBopGet(String key, long bkey,
Copy link
Contributor

Choose a reason for hiding this comment

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

660에서 long bkey 파라미터는 한줄 띄워져 있는데 여기는 key와 함께 붙어있네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

기존 구현에서도 두 표현방식이 달라 각각 기존 구현과 동일하게 맞추었는데, 전부 bkey 한줄 띄우도록 수정하겠습니다.

@@ -1288,6 +1316,39 @@ public <T> CollectionFuture<Map<ByteArrayBKey, Element<T>>> asyncBopGet(
dropIfEmpty, tc);
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

tc가 파라미터가 포함된 method 전에 이 method 들이 있어야 하지 않을까요?

구현쪽도 그에 맞게 순서가 변경되어야 할 것 같습니다.

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.

review 완료

public CollectionFuture<Map<ByteArrayBKey, Element<Object>>> asyncBopGet(
String key, byte[] from, byte[] to, ElementFlagFilter eFlagFilter,
int offset, int count) {
return asyncBopGet(key, from, to, eFlagFilter, offset, count);
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기 호출 시의 인자로, false, false가 추가되어야 합니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

위 코멘트가 Files changed 항목에서는 보이지 않는 데, github 버그인 것 같습니다.
위 코멘트를 확인해 주세요.

boolean reverse = BTreeUtil.compareByteArraysInLexOrder(from, to) > 0;
return asyncBopExtendedGet(key, get, reverse, tc);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

여기 코드에서는 API 코드 위치를 변경하지 말고,
추가할 API만 넣으면 리뷰하기 편할 것 같습니다.

public CollectionFuture<Map<ByteArrayBKey, Element<Object>>> asyncBopGet(
String key, byte[] from, byte[] to, ElementFlagFilter eFlagFilter,
int offset, int count) {
return asyncBopGet(key, from, to, eFlagFilter, offset, count);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@oliviarla
여기 false, false 인자가 추가되어야 합니다.

@jhpark816
Copy link
Collaborator

@oliviarla
CI test 실패를 확인해 주세요.

@oliviarla
Copy link
Collaborator Author

@jhpark816 CI 테스트 완료하였습니다.

@jhpark816 jhpark816 merged commit 670a4ba into naver:develop Jul 22, 2022
@oliviarla oliviarla deleted the asyncbopget branch September 4, 2024 07:55
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