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: Integration get and mget api #856

Closed
wants to merge 1 commit into from

Conversation

cheesecrust
Copy link
Collaborator

@cheesecrust cheesecrust commented Dec 26, 2024

🔗 Related Issue

⌨️ What I did

AS-IS
asyncGetBulk, asyncGetsBulk에서는 getOperationmgetOperation을 memcached 서버 버전에 맞게 생성하고 있습니다.

TO-BE
getOperation 만을 생성하고 안의 getOperationImpl 안에서 getmget을 판단하도록하여 두 APIType을 통합하였습니다.

따라서

  • get(s) operation 만을 생성하도록 수정
  • OperationFactoryget(s)의 인터페이스를 아래와 같이 변경하여 getmget을 구분하여 구현체를 생성하도록 하였습니다.
    get(s)(String key, GetOperation.Callback cb, boolean isMGet)
  • 더 이상 쓰이지 않는 Mget(s)OperationImpl은 삭제하였습니다.
  • 더 이상 쓰이지 않는 mget(s) 인터페이스 또한 삭제하였습니다.

@@ -179,18 +179,17 @@ public final void initialize() {

String keysString = generateKeysString();

if (cmd.equals("get") || cmd.equals("gets")) {
if (getHandlingNode() != null && !getHandlingNode().enabledMGetOp()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

initailize()가 호출되기 전에 handlingNode가 set 되지 않는 경우도 있나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

실제 opeation 동작시에는 initalize() 호출 전 handlingNode가 set 되지 않는 경우는 없습니다.
하지만, unit test node를 set하지 않고 initialize()를 호출하는 경우가 있어 이를 통과 하기 위해 null 검사 조건을 추가 하였습니다.

 @Test
  void getsTest() {
    try {
      opFact.gets(MULTIBYTE_KEY, new GetsOperation.Callback() {
        @Override
        public void receivedStatus(OperationStatus status) {
        }

        @Override
        public void complete() {
        }

        @Override
        public void gotData(String key, int flags, long cas, byte[] data) {
        }
      }).initialize();        // BaseGetOpImpl.initialize()
    } catch (java.nio.BufferOverflowException e) {
      fail();
    }

추가로 null 인 경우 gets나 mgets 을 수행하도록 한 이유는 기존 코드에서

if (node == null) {
        op = opFact.mgets(keyList, cb);
}

위 처럼 null 인 경우 mgets이나 gets명령을 수행하도록 하고 있기 때문입니다.

Copy link
Collaborator Author

@cheesecrust cheesecrust Dec 30, 2024

Choose a reason for hiding this comment

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

하지만 위에 말씀하신 방법
https://github.com/naver/arcus-java-client/pull/856/files#r1898412137
과 같이 한다면 기존 코드 변경 없이 진행 할 수 있을것 같습니다.

@@ -9,12 +9,13 @@
/**
* Operation for retrieving data.
*/
@Deprecated
Copy link
Collaborator

Choose a reason for hiding this comment

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

접근 제어자가 public인 부분이 염려되긴합니다.
다만 제 생각은 @Deprecated 하지 않고 바로 삭제해도 될 것 같습니다.

op = node.enabledMGetOp() ? opFact.mget(keyList, cb)
: opFact.get(keyList, cb);
}
op = opFact.get(keyList, cb);
Copy link
Collaborator

Choose a reason for hiding this comment

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

public GetOperation get(String key, GetOperation.Callback cb, boolean isMGet) {
    return new GetOperationImpl(key, cb, isMGet);
  }

위와 같이 인자 하나를 추가 시킨 다음 "get"과 "mget" 스트링 값을
GetOperationImpl 내에서 할당시켜주는게 좋을 거 같습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

다만 위 코멘트도 하휘호환성 문제가 있긴한데 제 생각은 @Deprecated 하지 않아도 될 것 같아요

@cheesecrust cheesecrust force-pushed the internal/getOperation branch from 2fed732 to 2dd5adc Compare December 30, 2024 02:55
@cheesecrust cheesecrust marked this pull request as draft December 30, 2024 02:57
@cheesecrust cheesecrust force-pushed the internal/getOperation branch 5 times, most recently from 446ffce to f3577fd Compare December 30, 2024 06:57
@cheesecrust cheesecrust marked this pull request as ready for review December 30, 2024 07:07
@cheesecrust
Copy link
Collaborator Author

#857
위 pr로 이전하여 close 합니다.

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