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

ENHANCE: Change creating callback objects multiple times in pipe operation. #727

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

brido4125
Copy link
Collaborator

관련 이슈

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

작업 내용

Piped 연산 시 op 인스턴스는 여러개 생성된다.
이와 동시에 callback 인스턴스도 여러개 생성된다.

하나의 콜백 인스턴스를 복수개의 op 인스턴스가 공유해서 사용하도록 변경한다.

@brido4125 brido4125 self-assigned this Mar 7, 2024
@brido4125 brido4125 force-pushed the enhance/pipeCallback branch from 4833d79 to 556db6e Compare March 7, 2024 07:39
@brido4125 brido4125 requested review from uhm0311 and oliviarla March 13, 2024 00:03
src/main/java/net/spy/memcached/ArcusClient.java Outdated Show resolved Hide resolved
src/main/java/net/spy/memcached/ArcusClient.java Outdated Show resolved Hide resolved
@brido4125 brido4125 force-pushed the enhance/pipeCallback branch 3 times, most recently from 5b3ae0e to 00e791f Compare March 18, 2024 06:14
@brido4125 brido4125 force-pushed the enhance/pipeCallback branch from 00e791f to 1ef9240 Compare March 19, 2024 02:27
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.

리뷰 완료

src/main/java/net/spy/memcached/ArcusClient.java Outdated Show resolved Hide resolved
@brido4125 brido4125 force-pushed the enhance/pipeCallback branch from 1ef9240 to df7b7e0 Compare March 25, 2024 05:56
@brido4125 brido4125 marked this pull request as draft April 2, 2024 02:53
@brido4125 brido4125 force-pushed the enhance/pipeCallback branch from df7b7e0 to b205d6b Compare April 2, 2024 02:53
@brido4125 brido4125 marked this pull request as ready for review April 2, 2024 02:56
@brido4125 brido4125 requested review from uhm0311 and oliviarla April 2, 2024 02:58
@brido4125 brido4125 force-pushed the enhance/pipeCallback branch from b205d6b to 24dd127 Compare April 2, 2024 04:50
@uhm0311 uhm0311 force-pushed the develop branch 3 times, most recently from 07918e6 to ae2a82f Compare April 2, 2024 07:53
@brido4125 brido4125 requested a review from uhm0311 April 3, 2024 02:35
@brido4125 brido4125 requested a review from jhpark816 April 3, 2024 02:44
@oliviarla
Copy link
Collaborator

@brido4125 현재 커밋이 3개입니다. rebase해주셔야 할 것 같습니다.

@brido4125 brido4125 force-pushed the enhance/pipeCallback branch from 24dd127 to eb87cef Compare April 5, 2024 02:31
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.

리뷰 완료

@jhpark816
Copy link
Collaborator

pipe 연산의 처리, 활용 방식을 정리할 때까지 본 PR 처리는 연기해야 할 것 같습니다.

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.

리뷰 완료

최신 코드 기준으로 rebase 해 주세요.

super(itemCount);
this.opIdx = opIdx;
Copy link
Collaborator

Choose a reason for hiding this comment

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

key 설정하는 문장 아래로 옮깁시다.

}

@Override
public void gotStatus(Integer index, int opIdx, OperationStatus status) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

인자 타입이 Integer, int가 혼용 사용되고 있는 데, int 타입으로 통일하면 될 것 같습니다.

참고로, 아래와 같이 구현할 수 있습니다.

      public void gotStatus(int opIdx, int elemIdx, OperationStatus status) {
        int index = (opIdx * CollectionPipedUpdate.MAX_PIPED_ITEM_COUNT) + elemIdx;
        if (status instanceof CollectionOperationStatus) {
          rv.addEachResult(index, (CollectionOperationStatus) status);
        } else {
          rv.addEachResult(index, new CollectionOperationStatus(status));
        }
      }

Copy link
Collaborator

Choose a reason for hiding this comment

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

아니면, gotStatus()은 index 인자를 받고,
해당되는 index 값은 gotStatus() 호출하는 쪽에서 계산하여 넘기는 것이 나을 것 같습니다.

      public void gotStatus(int index, OperationStatus status) {
        if (status instanceof CollectionOperationStatus) {
          rv.addEachResult(index, (CollectionOperationStatus) status);
        } else {
          rv.addEachResult(index, new CollectionOperationStatus(status));
        }
      }

@@ -1778,15 +1777,11 @@ public <T> CollectionFuture<Map<Integer, CollectionOperationStatus>> asyncBopPip
}

List<CollectionPipedInsert<T>> insertList = new ArrayList<CollectionPipedInsert<T>>();
PartitionedMap<Long, T> list = new PartitionedMap<Long, T>(elements,
Copy link
Collaborator

Choose a reason for hiding this comment

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

PartitionedMap 구현은 주어진 map 크기가 주어진 size 보다 작거나 같을 경우, 해당 map을 그대로 리턴하도록 최적화합시다.

insertList.add(new BTreePipedInsert<T>(key, elementMap, attributesForCreate, tc));
}
for (int i = 0; i < list.size(); i++) {
insertList.add(new BTreePipedInsert<T>(key, list.get(i), i, attributesForCreate, tc));
Copy link
Collaborator

Choose a reason for hiding this comment

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

인자 순서 검토 바람: list.get(i), i => i, list.get(i)

@brido4125
Copy link
Collaborator Author

pipe 연산의 처리, 활용 방식을 정리할 때까지 본 PR 처리는 연기해야 할 것 같습니다.

@jhpark816

PR 목록을 살펴보니 아직 pipe 관련된 변경사항이 merge 되진 않은 것 같습니다.
@oliviarla 님이 진행중인 작업이 마무리된 다음에 수정사항 반영해서 작업 하는게 좋을 것 같네요

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.

4 participants