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.

Sign up for GitHub

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

Jump to bottom

FIX: getElement logic in BTreeStoreAndGetFuture. #662

Merged
merged 1 commit into from
Sep 11, 2023
Merged

FIX: getElement logic in BTreeStoreAndGetFuture. #662

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/main/java/net/spy/memcached/internal/BTreeStoreAndGetFuture.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,12 @@
package net.spy.memcached.internal;

import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicReference;

import net.spy.memcached.OperationTimeoutException;
import net.spy.memcached.collection.Element;
import net.spy.memcached.ops.CollectionGetOpCallback;

Expand All @@ -41,6 +45,15 @@ public BTreeStoreAndGetFuture(CountDownLatch l, AtomicReference<T> oref, long op
}

public Element<E> getElement() {
try {
super.get(super.timeout, TimeUnit.MILLISECONDS);
} catch (ExecutionException e) {
throw new RuntimeException(e);
} catch (InterruptedException e) {
throw new RuntimeException(e);
} 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.

이 코드를 가장 상위에 두면 좋겠습니다.
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
나도 같은 의견이니, 이렇게 수정해 주시죠.

CollectionGetOpCallback callback = (CollectionGetOpCallback) op.getCallback();
callback.addResult();
return element;
Expand Down
Loading