INTERNAL: make piped insert operations process synchronously by oliviarla · Pull Request #887 · naver/arcus-java-client

oliviarla · 2025-03-14T08:20:27Z

🔗 Related Issue

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

⌨️ What I did

ArcusClient 클래스
- syncCollectionPipedInsert() / syncCollectionPipedUpdate() 메서드
  - 현재 수행중인 op 한 개가 complete된 후에 다음 op를 MemcachedNode가 처리할 수 있도록 등록한다.
PipedCollectionFuture
- 초기에 생성된 모든 operation들을 List 필드에 담아두도록 한다.
- 사용자로부터 cancel 요청 시, 기존에 cancel 되지 않은 상태라면 op를 순회하며 cancel을 시도한다.
- 내부적으로 혹은 사용자로부터 온 cancel 요청이 성공하지 않았거나, cancel 요청이 아직 진행중인 경우 isCancelled()는 false를 반환한다.
- 사용자의 cancel 요청(worker thread)과 내부 동작 중 cancel 요청(IO thread)이 동시에 발생할 수 있다.
  - BaseOperationImpl의 callbacked 변수로 인해 해당 값을 먼저 점유한 요청이 먼저 cancel을 시도하게 된다.
  - 이미 cancel된 후에는 다른 요청이 cancel을 할 수 없게 된다.
    - 사용자 요청이 실패했다면 Future에서 다음 op에 대해 cancel 요청을 보낼 것이다.
    - 내부 요청이 실패했다면 별다른 처리를 하지 않는다.

uhm0311

우선 질문 위주입니다.

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

src/main/java/net/spy/memcached/protocol/ascii/SingleKeyPipeOperationImpl.java

src/test/manual/net/spy/memcached/bulkoperation/BopPipeUpdateTest.java

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

jhpark816

1차 리뷰 의견입니다.

추가 리뷰를 바로 진행할겠습니다.

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

jhpark816

추가 리뷰

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

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

oliviarla

다른 리뷰 사항들은 반영해두었습니다.

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

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

jhpark816

일부 리뷰

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

oliviarla · 2025-03-27T07:22:00Z

@jhpark816 리뷰 반영하였습니다.

jhpark816

리뷰 완료

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

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

src/main/java/net/spy/memcached/protocol/ascii/PipeOperationImpl.java

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

jhpark816

리뷰 완료

src/main/java/net/spy/memcached/protocol/ascii/PipeOperationImpl.java

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

jhpark816 · 2025-04-02T01:35:45Z

@uhm0311 리뷰 바랍니다.

uhm0311

질문입니다.

src/test/manual/net/spy/memcached/bulkoperation/PipeInsertTest.java

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

src/test/manual/net/spy/memcached/bulkoperation/BopPipeUpdateTest.java

src/main/java/net/spy/memcached/protocol/ascii/PipeOperationImpl.java

src/test/manual/net/spy/memcached/bulkoperation/LopInsertBulkMultipleValueTest.java

src/test/manual/net/spy/memcached/bulkoperation/PipeInsertTest.java

jhpark816

일부 리뷰

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

src/main/java/net/spy/memcached/protocol/ascii/PipeOperationImpl.java

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

jhpark816 · 2025-08-13T02:54:32Z

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

      }
    }
-    return false;
+    return cancelled.get();


future의 cancel(), isCancelled()가 자주 호출되는 메소드가 아니므로,
cancelled AtomicBoolean 값을 따로 두지 않아도 될 것 같은데요.
그리고, 단순 Boolean 값이어도 될 것 같습니다. 검토 바랍니다.
다른 쪽의 bulk 연산에서는 이러한 cancelled 값을 유지하는 지 궁금합니다.

future의 cancel(), isCancelled()가 자주 호출되는 메소드가 아니므로,
cancelled AtomicBoolean 값을 따로 두지 않아도 될 것 같은데요.

PipedCollectionFuture는 모든 op를 취소하는 것이 아니라 특정 op가 취소 가능하다면 취소시키고 바로 결과를 반환합니다. 그리고 이 과정에서 cancelled 플래그를 true로 두어, 다시 cancel 메서드가 호출되었을 때 더이상 op를 취소하지 않도록 합니다.

단순 Boolean 값이어도 될 것 같습니다.

대부분의 상황에서 cancel과 isCancelled를 하나의 스레드에서만 호출하기 때문에 boolean 타입을 사용해도 괜찮긴 한데, 여러 스레드에서 동시에 호출되는 케이스는 고려하지 않아도 되나요?

다른 쪽의 bulk 연산에서는 이러한 cancelled 값을 유지하는 지

다른쪽 bulk 연산은 비동기 방식이기 때문에 비교하기가 애매합니다.

jhpark816 · 2025-08-13T03:03:07Z

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

+      public void complete() {
+        CollectionOperationStatus operationStatus = rv.getOperationStatus();
+        if (operationStatus != null && operationStatus.isSuccess()
+                && !ops.get(opIdx).isCancelled()


!ops.get(opIdx).isCancelled() 조건이 필요한 지 궁금합니다.
현재 연산이 cancel 되었으면, operatoinStatus.isSuccess() == false 상태이지 않는 지 ?

워커 스레드로부터 cancel이 호출되어 CANCELED 상태를 operationStatus에 저장하고 complete 메서드를 호출하는 도중에, IO 스레드에서 결과를 읽어들여 성공 상태로 덮어쓸 수 있습니다.

PipedCollectionFuture#setOperationStatus 메서드를 보시면 CANCELED 상태가 저장되어 있더라도 새로운 결과를 덮어쓰는 것을 확인할 수 있습니다.

jhpark816 · 2025-08-19T01:18:53Z

@oliviarla 리뷰 코멘트 달았으니, 확인 바랍니다.

oliviarla

나머지 코멘트에 대한 리뷰는 반영해두었습니다.

oliviarla · 2025-08-18T11:50:49Z

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

+      public void complete() {
+        CollectionOperationStatus operationStatus = rv.getOperationStatus();
+        if (operationStatus != null && operationStatus.isSuccess()
+                && !ops.get(opIdx).isCancelled()


워커 스레드로부터 cancel이 호출되어 CANCELED 상태를 operationStatus에 저장하고 complete 메서드를 호출하는 도중에, IO 스레드에서 결과를 읽어들여 성공 상태로 덮어쓸 수 있습니다.

PipedCollectionFuture#setOperationStatus 메서드를 보시면 CANCELED 상태가 저장되어 있더라도 새로운 결과를 덮어쓰는 것을 확인할 수 있습니다.

oliviarla · 2025-08-19T01:40:08Z

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

      }
    }
-    return false;
+    return cancelled.get();


future의 cancel(), isCancelled()가 자주 호출되는 메소드가 아니므로,
cancelled AtomicBoolean 값을 따로 두지 않아도 될 것 같은데요.

PipedCollectionFuture는 모든 op를 취소하는 것이 아니라 특정 op가 취소 가능하다면 취소시키고 바로 결과를 반환합니다. 그리고 이 과정에서 cancelled 플래그를 true로 두어, 다시 cancel 메서드가 호출되었을 때 더이상 op를 취소하지 않도록 합니다.

단순 Boolean 값이어도 될 것 같습니다.

대부분의 상황에서 cancel과 isCancelled를 하나의 스레드에서만 호출하기 때문에 boolean 타입을 사용해도 괜찮긴 한데, 여러 스레드에서 동시에 호출되는 케이스는 고려하지 않아도 되나요?

다른 쪽의 bulk 연산에서는 이러한 cancelled 값을 유지하는 지

다른쪽 bulk 연산은 비동기 방식이기 때문에 비교하기가 애매합니다.

src/main/java/net/spy/memcached/protocol/ascii/PipeOperationImpl.java

oliviarla force-pushed the pipe3 branch from 2953ef5 to 9cfa7b4 Compare March 17, 2025 05:25

oliviarla marked this pull request as ready for review March 17, 2025 05:25

oliviarla force-pushed the pipe3 branch from 9cfa7b4 to 50ed543 Compare March 17, 2025 05:49

oliviarla requested review from jhpark816 and uhm0311 March 17, 2025 05:49

uhm0311 reviewed Mar 17, 2025

View reviewed changes

oliviarla force-pushed the pipe3 branch from 50ed543 to 6805f4d Compare March 17, 2025 08:48

jhpark816 reviewed Mar 17, 2025

View reviewed changes

oliviarla force-pushed the pipe3 branch from 6805f4d to b33b557 Compare March 18, 2025 23:53

oliviarla commented Mar 18, 2025

View reviewed changes

src/main/java/net/spy/memcached/ArcusClient.java Show resolved Hide resolved

src/main/java/net/spy/memcached/internal/PipedCollectionFuture.java Outdated Show resolved Hide resolved

oliviarla force-pushed the pipe3 branch from b33b557 to fce6747 Compare March 19, 2025 05:43

oliviarla self-assigned this Mar 19, 2025

oliviarla force-pushed the pipe3 branch from fce6747 to f84c9cb Compare March 20, 2025 02:40

oliviarla requested review from jhpark816 and uhm0311 March 20, 2025 10:15

jhpark816 reviewed Mar 21, 2025

View reviewed changes

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

src/main/java/net/spy/memcached/ArcusClient.java Outdated Show resolved Hide resolved

oliviarla force-pushed the pipe3 branch 3 times, most recently from a7063fa to 2939657 Compare March 27, 2025 07:16

jhpark816 reviewed Mar 27, 2025

View reviewed changes

src/main/java/net/spy/memcached/ArcusClient.java Outdated Show resolved Hide resolved

oliviarla force-pushed the pipe3 branch from 2939657 to a711c34 Compare March 31, 2025 05:49

jhpark816 reviewed Mar 31, 2025

View reviewed changes

oliviarla force-pushed the pipe3 branch from a711c34 to a64e69c Compare April 1, 2025 02:03

uhm0311 reviewed Apr 2, 2025

View reviewed changes

src/test/manual/net/spy/memcached/bulkoperation/PipeInsertTest.java Outdated Show resolved Hide resolved

src/main/java/net/spy/memcached/ArcusClient.java Outdated Show resolved Hide resolved

oliviarla force-pushed the pipe3 branch 2 times, most recently from ac87315 to cf6c6fa Compare April 2, 2025 09:09

oliviarla force-pushed the pipe3 branch from 0d2de44 to 7de8a1d Compare April 23, 2025 07:45

This comment was marked as outdated.

Sign in to view

oliviarla marked this pull request as draft May 2, 2025 00:46

oliviarla force-pushed the pipe3 branch from 7de8a1d to 0efb0cb Compare June 10, 2025 07:49

oliviarla marked this pull request as ready for review June 10, 2025 07:49

uhm0311 reviewed Jun 11, 2025

View reviewed changes

oliviarla force-pushed the pipe3 branch 2 times, most recently from 6c6ba28 to ab39e1e Compare July 30, 2025 12:15

oliviarla marked this pull request as draft July 30, 2025 12:18

oliviarla force-pushed the pipe3 branch from ab39e1e to d62abf0 Compare July 30, 2025 12:25

oliviarla marked this pull request as ready for review July 30, 2025 12:40

oliviarla requested a review from Copilot July 30, 2025 12:42

This comment was marked as resolved.

Sign in to view

oliviarla force-pushed the pipe3 branch from d62abf0 to ba28565 Compare July 30, 2025 12:54

oliviarla requested a review from uhm0311 July 30, 2025 12:57

oliviarla mentioned this pull request Jul 30, 2025

INTERNAL: Set OperationStatus even if already CANCELED status exists #939

Merged

uhm0311 reviewed Jul 31, 2025

View reviewed changes

src/test/manual/net/spy/memcached/bulkoperation/LopInsertBulkMultipleValueTest.java Show resolved Hide resolved

src/test/manual/net/spy/memcached/bulkoperation/PipeInsertTest.java Show resolved Hide resolved

oliviarla force-pushed the pipe3 branch from ba28565 to 300c10f Compare July 31, 2025 03:08

uhm0311 previously approved these changes Jul 31, 2025

View reviewed changes

oliviarla requested a review from jhpark816 July 31, 2025 06:58

oliviarla force-pushed the pipe3 branch from 300c10f to 344e103 Compare August 6, 2025 10:03

jhpark816 reviewed Aug 13, 2025

View reviewed changes

INTERNAL: Make piped insert operations process synchronously

518249e

oliviarla dismissed uhm0311’s stale review via 518249e August 19, 2025 02:55

oliviarla force-pushed the pipe3 branch from 344e103 to 518249e Compare August 19, 2025 02:55

oliviarla commented Aug 19, 2025

View reviewed changes

jhpark816 approved these changes Aug 19, 2025

View reviewed changes

jhpark816 merged commit e9c7897 into naver:develop Aug 19, 2025
2 checks passed

oliviarla deleted the pipe3 branch August 28, 2025 08:17

Comments

Conversation

oliviarla commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Related Issue

⌨️ What I did

Uh oh!

uhm0311 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oliviarla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oliviarla commented Mar 27, 2025

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhpark816 commented Apr 2, 2025

Uh oh!

uhm0311 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oliviarla commented Mar 14, 2025 •

edited

Loading

jhpark816 Aug 13, 2025 •

edited

Loading