ENHANCE: change put method logic in LocalCacheManager in aysncGet. #622

brido4125 · 2023-06-16T07:26:19Z

https://github.com/jam2in/arcus-works/issues/406 관련 PR입니다.
asyncGet 호출 시 put() 로직 수행을 IO 쓰레드에서 응용의 워커 쓰레드로 변경하였습니다.

jhpark816

리뷰 완료

jhpark816 · 2023-06-16T09:11:39Z

src/main/java/net/spy/memcached/internal/GetFuture.java

    this.rv = new OperationFuture<Future<T>>(l, opTimeout);
+    this.localCacheManager = localCacheManager;
+    this.key = key;


GetFuture 생성자는 그대로 유지하고,
localCacheManager와 key를 주입하는 별도 메소드를 제공합시다.

그리고, FrontCacheMemcachedClient 클래스의 asyncGet() 메소드를 아래와 같이 수정합시다. 동작하는 코드인지는 확인하지 않았습니다.

public <T> GetFuture<T> asyncGet(final String key, final Transcoder<T> tc) { if (localCacheManager != null) { Element frontElement = localCacheManager.getElement(key); if (frontElement != null) { return new FrontCacheGetFuture<T>(frontElement); } } GetFuture<T> rv = super.asyncGet(key, tc); rv.setLocalCacheManager(localCacheManger, key); return rv; }

최대한 MemcachedClient 에는 localCacheManager 기능이 들어가지 않도록 하기 위함입니다.

참고로, asyncGetBulk()도 위와 같은 방식으로 변경하면,
기존 MemcachedClient에서 local cache 사항을 모두 제거할 수 있습니다.

만약 localCacheManager와 key를 주입하는 별도의 setter를 두게 될 경우 해당 메서드가 응용의 사용자에게 노출되는 문제점이 발생합니다.

현재 구조의 경우에도 MemcachedClient의 asyncGet 메서드는 localCacheManager를 생성자 주입 로직에서만 사용합니다.

asyncGetBulk와 같이 MemcachedClient에서 비지니스 로직에 localCacheManager가 사용되면 수정되어야 하지만 생성자 주입 정도의 로직은 캡슐화의 관점에서는 수행되어도 괜찮다고 생각됩니다.

@brido4125
해당 setter 메소드가 노출되는 문제가 염려된다면, GetFuture 상속받는 FrontCacheGetFuture를 두어 사용하면 될 것 같습니다. 기존에 FrontCachtGetFuture 존재하는 데, 이는 어떤 용도인지 확인하지 않았지만, 기존 용도 외에 위의 setter 메소드를 가지는 용도로 확장하면 되지 않나 생각합니다.

변경 검토 해보았습니다.
FrontCachtGetFuture 는 asyncGet 요청이 localCache에 cache-hit이 되었을 경우 리턴하는 Future타입입니다.

MemcachedClient에서 해당 Future를 생성할 경우 asyncGetBulk API가 항상 FrontCache를 사용하는 api인 것처럼 코드가 읽히기 됩니다.

그래서 현재 용도와 setter 메서드를 가지는 용도를 합치게 하여 FrontCacheMemcachedClient와 MemcachedClienet에서 모두 해당 Future를 사용하게 하는것은 의미적으로 불분명한 변경이라고 생각됩니다.

jhpark816 · 2023-06-16T09:11:54Z

src/main/java/net/spy/memcached/MemcachedClient.java

@@ -930,9 +930,6 @@ public void gotData(String k, int flags, byte[] data) {

          public void complete() {
            // FIXME weird...


FIXME 주석도 함께 제거

jhpark816 · 2023-06-19T02:28:00Z

@uhm0311 @oliviarla 본 PR 확인해 두고, 필요하면 리뷰하세요.

uhm0311 · 2023-06-19T02:36:42Z

src/main/java/net/spy/memcached/MemcachedClient.java

@@ -912,7 +912,7 @@ public OperationFuture<Boolean> replace(String key, int exp, Object o) {
  public <T> GetFuture<T> asyncGet(final String key, final Transcoder<T> tc) {

    final CountDownLatch latch = new CountDownLatch(1);
-    final GetFuture<T> rv = new GetFuture<T>(latch, operationTimeout);
+    final GetFuture<T> rv  = new GetFuture<T>(latch, operationTimeout, localCacheManager, key);


의미 없는 공백 제거해주세요.

brido4125 · 2023-06-20T03:24:19Z

현재 MemcachedClient에 존재하는 localCacheManager field값은 asyncGetBulk 관련 PR에서 완전히 제거할 예정입니다.

jhpark816 · 2023-06-20T06:44:21Z

@brido4125 수정이 완료된 상태인가요?

brido4125 · 2023-06-20T08:25:53Z

@jhpark816 네 수정된 상태입니다.

jhpark816 · 2023-06-20T08:26:46Z

@uhm0311 @oliviarla 리뷰 바랍니다.

uhm0311 · 2023-06-20T10:31:54Z

src/main/java/net/spy/memcached/internal/GetFuture.java

@@ -30,21 +40,28 @@ public boolean cancel(boolean ign) {

  public T get() throws InterruptedException, ExecutionException {
    Future<T> v = rv.get();
-    return v == null ? null : v.get();
+    if (v == null) {
+      //여기로 호출


이건 무엇인가요?

디버깅하다가 남은 주석입니다. 삭제하겠습니다

brido4125 · 2023-06-26T09:12:08Z

src/main/java/net/spy/memcached/plugin/FrontCacheMemcachedClient.java

-
-    if (localCacheManager != null) {
-      frontElement = localCacheManager.getElement(key);
+    if (localCacheManager == null) {


FrontCache자체를 사용하지 않을 경우에는 Memcached의 asynGet 호출

brido4125 · 2023-06-26T09:16:19Z

FrontCacheGetFuture 사용 용도 변경 코멘트

기존 : FrontCache가 Hit일 경우 FrontCacheGetFuture를 리턴하도록 하면서 사용
변경 : FrontCacheGetFuture는 Cache Miss일 경우 사용된다. MemcachedClient의 aysncGet의 반환값인 GetFuture를 인수로 받는다.또한 응용으로부터 get()이 호출될 경우LocalCacheManager를 사용하며 put() 로직을 수행한다.

리뷰 시 참고 부탁드립니다.

brido4125 · 2023-06-26T09:19:15Z

src/main/java/net/spy/memcached/internal/GetFuture.java

+    this.opTimeout = opTimeout;
+  }
+
+  public GetFuture(OperationFuture<Future<T>> rv, CountDownLatch l, long opTimeout) {


자식 클래스인 FrontCacheGetFuture를 생성하기 위한 생성자 추가

아래와 같이 OperationFutue 객체만 인자로 받아 설정하는 생성자만 두고,
latch, opTimeout 필드는 제거해도 될 것 같습니다.

public GetFuture(OperationFuture<Future<T>> rv) { this.rv = rv; }

jhpark816 · 2023-06-28T02:57:02Z

@uhm0311 @oliviarla 리뷰 바랍니다.

oliviarla · 2023-06-29T08:50:35Z

src/main/java/net/spy/memcached/plugin/FrontCacheMemcachedClient.java

-    } else {
-      return new FrontCacheGetFuture<T>(frontElement);
+    final T t = localCacheManager.get(key, tc);
+    if (t != null) {


if(t==null) 로 짧은 로직 먼저 처리하고 return하는건 어떤가요?

jhpark816

리뷰 완료

jhpark816 · 2023-06-29T09:31:13Z

src/main/java/net/spy/memcached/internal/GetFuture.java

+    this.opTimeout = opTimeout;
+  }
+
+  public GetFuture(OperationFuture<Future<T>> rv, CountDownLatch l, long opTimeout) {


아래와 같이 OperationFutue 객체만 인자로 받아 설정하는 생성자만 두고,
latch, opTimeout 필드는 제거해도 될 것 같습니다.

public GetFuture(OperationFuture<Future<T>> rv) { this.rv = rv; }

jhpark816 · 2023-06-29T10:41:36Z

src/main/java/net/spy/memcached/plugin/FrontCacheGetFuture.java

-  @Override
-  public boolean cancel(boolean mayInterruptIfRunning) {
-    return false;
+  public FrontCacheGetFuture(GetFuture<T> parent, LocalCacheManager localCacheManager, String key) {


아래 순서로 인자를 나열하시죠.

localCacheManager

key

parent (parent 대신에 getFuture 라는 용어가 나은 것 같음)

value 얻기 위한 future

jhpark816 · 2023-06-29T10:43:14Z

src/main/java/net/spy/memcached/plugin/FrontCacheGetFuture.java

-  public boolean cancel(boolean mayInterruptIfRunning) {
-    return false;
+  public FrontCacheGetFuture(GetFuture<T> parent, LocalCacheManager localCacheManager, String key) {
+    super(parent.getRv(), parent.getLatch(), parent.getOpTimeout());


위에서 같이 OperationFuture 객체를 인자로 받는 GetFuture 생성자를 추가한다고 가정하고,
여기서는 super(getFuture); 형태로 호출하면 문제가 있나요?

아.. GetFuture가 OperationFuture 상속하는 관계가 아니어서,
OperationFuture 새로 생성하는 방식을 사용한 것이군요.

그러면, super(getFuture.getRv()) 형태로 호출해야 하겠군요.

jhpark816

리뷰 완료

jhpark816 · 2023-06-30T03:52:35Z

src/main/java/net/spy/memcached/plugin/FrontCacheMemcachedClient.java

-    } else {
-      return new FrontCacheGetFuture<T>(frontElement);
+    final T t = localCacheManager.get(key, tc);
+    if (t == null) {


이건 원래대로 if (t != null) 조건을 사용하는 것이 좋습니다.

jhpark816 · 2023-06-30T03:54:29Z

src/main/java/net/spy/memcached/internal/GetFuture.java

@@ -19,32 +19,40 @@
 public class GetFuture<T> implements Future<T> {

  private final OperationFuture<Future<T>> rv;
+  private CountDownLatch latch;
+  private long opTimeout;


2개 필드는 없어도 됩니다.

기존의 GetFuture를 생성하는 memcachedCilent의 asyncGet에서 해당 필드가 필요합니다.

asyncGet()의 어디서 필요하나요?

asyncGet() api내의 GetFuture의 생성자에서 필요합니다.

GetFuture의 생성자에서 위 두 필드를 통해 OperationFuture의 생성자를 호출할때 사용합니다.

MemcachedClient.asyncGet()에서 latch 생성하고, operationTimeout은 내부 필드 값 사용합니다.

public <T> GetFuture<T> asyncGet(final String key, final Transcoder<T> tc) { final CountDownLatch latch = new CountDownLatch(1); final GetFuture<T> rv = new GetFuture<T>(latch, operationTimeout); . . . }

Latch를 Future에서 생성하도록 변경하겠습니다.

latch를 future에서 생성하지 않아도 될 것 같습니다.

jhpark816 · 2023-06-30T03:57:42Z

src/main/java/net/spy/memcached/plugin/FrontCacheGetFuture.java

-  public boolean cancel(boolean mayInterruptIfRunning) {
-    return false;
+  public FrontCacheGetFuture(LocalCacheManager localCacheManager, String key, GetFuture<T> parent) {
+    super(parent.getRv());


아래 형태로 수행되게 하려면 어떤 수정이 추가로 필요한가요?

super(parent)

jhpark816 · 2023-07-03T03:10:08Z

@uhm0311 @oliviarla 다시 리뷰 바랍니다.

jhpark816

리뷰 완료

jhpark816 · 2023-07-03T03:36:21Z

src/main/java/net/spy/memcached/plugin/FrontCacheMemcachedClient.java

@@ -67,6 +68,9 @@ public FrontCacheMemcachedClient(ConnectionFactory cf,
    }
  }

+  public GetFuture<Object> asyncGet(final String key) {
+    return asyncGet(key, transcoder);
+  }


아래에 1개 empty line 추가

위에 메소드 주석 추가

get() 메소드도 지원해야 할 것 같습니다.

get() 메소드도 지원해야 할 것 같습니다.

FrontCacheGetFuture 내에 get()에 대한 구현체가 있습니다.
어떤 get() 메서드를 말씀하시는걸까요?

MemcachedClient.get() 메소드에 대응하는
FrontCacheMemcachedClient.get() 메소드가 있어야 할 것 같습니다.

brido4125 requested a review from jhpark816 June 16, 2023 08:51

jhpark816 reviewed Jun 16, 2023

View reviewed changes

brido4125 force-pushed the enhance/asyncGetDecode branch from 19b0934 to 8450195 Compare June 19, 2023 02:06

brido4125 self-assigned this Jun 19, 2023

jhpark816 requested review from oliviarla and uhm0311 June 19, 2023 02:27

uhm0311 reviewed Jun 19, 2023

View reviewed changes

brido4125 force-pushed the enhance/asyncGetDecode branch 2 times, most recently from 9c68ec5 to a336d3a Compare June 20, 2023 03:14

uhm0311 reviewed Jun 20, 2023

View reviewed changes

brido4125 force-pushed the enhance/asyncGetDecode branch 2 times, most recently from 49f7528 to 401b6a2 Compare June 21, 2023 03:33

brido4125 commented Jun 26, 2023

View reviewed changes

brido4125 force-pushed the enhance/asyncGetDecode branch 2 times, most recently from de635d3 to cce8e3e Compare June 26, 2023 10:06

uhm0311 approved these changes Jun 29, 2023

View reviewed changes

oliviarla approved these changes Jun 29, 2023

View reviewed changes

jhpark816 reviewed Jun 29, 2023

View reviewed changes

brido4125 force-pushed the enhance/asyncGetDecode branch from cce8e3e to 988297b Compare June 30, 2023 03:49

jhpark816 reviewed Jun 30, 2023

View reviewed changes

brido4125 force-pushed the enhance/asyncGetDecode branch 2 times, most recently from 6c8663c to 7e7353f Compare July 3, 2023 02:37

jhpark816 requested review from oliviarla and uhm0311 July 3, 2023 03:09

uhm0311 approved these changes Jul 3, 2023

View reviewed changes

jhpark816 reviewed Jul 3, 2023

View reviewed changes

ENHANCE: change put method logic in LocalCacheManager in aysncGet.

da1b831

brido4125 force-pushed the enhance/asyncGetDecode branch from 7e7353f to da1b831 Compare July 3, 2023 06:22

brido4125 mentioned this pull request Jul 4, 2023

ENHANCE: Seperate FrontCacheLogic in asyncGetBulk from MemcachedClient to FrontCacheClient. #624

Merged

oliviarla approved these changes Jul 4, 2023

View reviewed changes

brido4125 requested a review from jhpark816 July 5, 2023 11:34

jhpark816 merged commit 2f03ed7 into naver:develop Jul 5, 2023
1 check passed

brido4125 deleted the enhance/asyncGetDecode branch July 6, 2023 00:14

		@@ -930,9 +930,6 @@ public void gotData(String k, int flags, byte[] data) {

		public void complete() {
		// FIXME weird...

ENHANCE: change put method logic in LocalCacheManager in aysncGet. #622

ENHANCE: change put method logic in LocalCacheManager in aysncGet. #622

Conversation

brido4125 commented Jun 16, 2023 • edited Loading

jhpark816 left a comment

Choose a reason for hiding this comment

jhpark816 Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

brido4125 Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 commented Jun 19, 2023

Choose a reason for hiding this comment

brido4125 commented Jun 20, 2023

jhpark816 commented Jun 20, 2023

brido4125 commented Jun 20, 2023

jhpark816 commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brido4125 commented Jun 26, 2023

FrontCacheGetFuture 사용 용도 변경 코멘트

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 commented Jun 28, 2023

Choose a reason for hiding this comment

jhpark816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

jhpark816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 commented Jul 3, 2023

jhpark816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brido4125 commented Jun 16, 2023 •

edited

Loading

jhpark816 Jun 16, 2023 •

edited

Loading

jhpark816 Jun 19, 2023 •

edited

Loading

brido4125 Jun 19, 2023 •

edited

Loading

jhpark816 Jun 29, 2023 •

edited

Loading