INTERNAL: return partial result if error or cancel occurred in getSome method of BulkGetFuture #808

oliviarla · 2024-09-19T11:13:13Z

🔗 Related Issue

⌨️ What I did

getSome()에서 Error, Cancel이 발생하더라도 조회가 완료된 결과를 리턴하도록 변경했습니다.
InterruptException이 발생한 경우에는 그대로 반환합니다.

jhpark816

리뷰 완료

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

brido4125 · 2024-09-19T23:50:05Z

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

    try {
      return internalGet(duration, unit, false);
-    } catch (TimeoutException e) {
+    } catch (TimeoutException | ExecutionException e) {
      throw new AssertionError("Something went wrong...", e);


Timeout과 Execution을 별도 메세지로 처리하면 좋을 것 같아요

제 생각에는 timeout과 execution 예외는 internalGet(duration, unit, false) 메서드를 코드대로 수행 시 발생하지 않기 때문에 별도 메시지로 처리할 필요까지는 없을 것 같습니다.

아니면 말씀하신대로 해당 예외는 발생하지 않는다라는 에러 메세지를 남기는게 좋을 것 같네요.
"Something went wrong"은 너무 추상적이라고 생각됩니다.

또는 ~~예외 대신 log.error~~ 주석을 통해 ureachable 한 로직이라는 것을 명시하는것도 좋을것 같습니다.

더 디테일한 메시지로 변경했습니다.

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

brido4125 · 2024-09-20T02:28:16Z

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

    try {
      return internalGet(duration, unit, false);
-    } catch (TimeoutException e) {
+    } catch (TimeoutException | ExecutionException e) {
      throw new AssertionError("Something went wrong...", e);


아니면 말씀하신대로 해당 예외는 발생하지 않는다라는 에러 메세지를 남기는게 좋을 것 같네요.
"Something went wrong"은 너무 추상적이라고 생각됩니다.

또는 ~~예외 대신 log.error~~ 주석을 통해 ureachable 한 로직이라는 것을 명시하는것도 좋을것 같습니다.

…e method of BulkGetFuture

oliviarla mentioned this pull request Sep 19, 2024

INTERNAL: return CompositeException from get method in BulkGetFuture #806

Merged

oliviarla requested review from jhpark816 and brido4125 September 19, 2024 12:12

jhpark816 reviewed Sep 19, 2024

View reviewed changes

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

brido4125 reviewed Sep 19, 2024

View reviewed changes

oliviarla changed the title ~~INTERNAL: return partial result if error or cancel occurred in getSom…~~ INTERNAL: return partial result if error or cancel occurred in getSome method of BulkGetFuture Sep 20, 2024

oliviarla force-pushed the getsome branch from 3176cb6 to c69168c Compare September 20, 2024 02:15

brido4125 previously approved these changes Sep 20, 2024

View reviewed changes

INTERNAL: return partial result if error or cancel occurred in getSom…

d5e5fd7

…e method of BulkGetFuture

oliviarla dismissed brido4125’s stale review via d5e5fd7 September 20, 2024 02:36

oliviarla force-pushed the getsome branch from c69168c to d5e5fd7 Compare September 20, 2024 02:36

oliviarla requested a review from jhpark816 September 20, 2024 03:16

jhpark816 approved these changes Sep 20, 2024

View reviewed changes

jhpark816 merged commit 7c3f002 into naver:develop Sep 20, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTERNAL: return partial result if error or cancel occurred in getSome method of BulkGetFuture #808

INTERNAL: return partial result if error or cancel occurred in getSome method of BulkGetFuture #808

oliviarla commented Sep 19, 2024 •

edited

Loading

jhpark816 left a comment

brido4125 Sep 19, 2024

oliviarla Sep 20, 2024

brido4125 Sep 20, 2024 •

edited

Loading

oliviarla Sep 20, 2024

brido4125 Sep 20, 2024 •

edited

Loading

INTERNAL: return partial result if error or cancel occurred in getSome method of BulkGetFuture #808

INTERNAL: return partial result if error or cancel occurred in getSome method of BulkGetFuture #808

Conversation

oliviarla commented Sep 19, 2024 • edited Loading

🔗 Related Issue

⌨️ What I did

jhpark816 left a comment

Choose a reason for hiding this comment

brido4125 Sep 19, 2024

Choose a reason for hiding this comment

oliviarla Sep 20, 2024

Choose a reason for hiding this comment

brido4125 Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

oliviarla Sep 20, 2024

Choose a reason for hiding this comment

brido4125 Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

oliviarla commented Sep 19, 2024 •

edited

Loading

brido4125 Sep 20, 2024 •

edited

Loading

brido4125 Sep 20, 2024 •

edited

Loading