Skip to content

Conversation

@wunameya
Copy link
Contributor

@wunameya wunameya commented Sep 7, 2025

Support custom retryable exceptions via @RetryableException.
Fixes #1362

Summary by CodeRabbit

  • New Features
    • Support for application-defined retriable exceptions: server-side exceptions marked as retryable will trigger automatic client retries under the existing retry policy.
    • New dedicated error type for application-triggered retries to improve logging and observability.
    • Simple enablement: annotate your exception class to have failures retried without changing client code.

✏️ Tip: You can customize this high-level summary in your review settings.

@sofastack-cla sofastack-cla bot added cla:yes CLA is ok size/S labels Sep 7, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 7, 2025

Walkthrough

Adds a provider-side marker annotation and an error code, maps annotated exceptions to a new CUSTOMER_RETRY_ERROR on provider, and implements client-side retry when the response's app exception indicates CUSTOMER_RETRY_ERROR. No public method signatures changed.

Changes

Cohort / File(s) Summary of changes
Client failover retry handling
core-impl/client/src/main/java/com/alipay/sofa/rpc/client/FailoverCluster.java
After receiving a non-null response, checks response.getAppResponse() for a SofaRpcException with errorType == CUSTOMER_RETRY_ERROR; if found, increments retry counter and continues the retry loop to the next provider.
Provider-side retry mapping
core/api/src/main/java/com/alipay/sofa/rpc/filter/ProviderInvoker.java
On InvocationTargetException, inspects the underlying cause; if the cause's class is annotated with @RetryableException, wraps it into a SofaRpcException with errorType = CUSTOMER_RETRY_ERROR as the app response, otherwise preserves the original cause as app response.
Retryable annotation
core/common/src/main/java/com/alipay/sofa/rpc/common/annotation/RetryableException.java
Adds new public marker annotation @RetryableException with @Documented, @Target(ElementType.TYPE), and @Retention(RetentionPolicy.RUNTIME).
Error type extension
core/exception/src/main/java/com/alipay/sofa/rpc/core/exception/RpcErrorType.java
Adds public static final int CUSTOMER_RETRY_ERROR = 300; to represent user-defined retriable exceptions.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant FailoverCluster
  participant ProviderInvoker
  participant ServiceImpl

  Client->>FailoverCluster: invoke(request)
  loop attempts (until max)
    FailoverCluster->>ProviderInvoker: invoke(request)
    ProviderInvoker->>ServiceImpl: call method
    alt Service throws annotated exception
      ServiceImpl-->>ProviderInvoker: InvocationTargetException(cause@RetryableException)
      ProviderInvoker-->>FailoverCluster: response with appResponse = SofaRpcException(CUSTOMER_RETRY_ERROR)
      FailoverCluster->>FailoverCluster: detect CUSTOMER_RETRY_ERROR, increment retry count
      FailoverCluster-->>ProviderInvoker: try next provider (loop)
    else Success or non-retryable error
      ServiceImpl-->>ProviderInvoker: return / non-retryable error
      ProviderInvoker-->>FailoverCluster: response/appResponse
      FailoverCluster-->>Client: return final response
    end
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Pay attention to exception wrapping logic in ProviderInvoker.invoke and how InvocationTargetException causes are cut/preserved.
  • Verify retry loop termination and counter increment in FailoverCluster.doInvoke to avoid infinite retries.
  • Ensure the new RpcErrorType.CUSTOMER_RETRY_ERROR is used consistently and documented where relevant.

Suggested reviewers

  • EvenLjj
  • chuailiwu

Poem

A rabbit finds a bug, then gives a hop and grin,
“Tag it retry!” — whispers soft, and sends the call again.
Providers wink with 300, clients roll the dice,
Through fields of hops and retries, we dance on code so nice. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: adding support for custom retryable exceptions via a new @RetryableException annotation, which matches the PR's core objective.
Linked Issues check ✅ Passed The PR implements the objective from #1362 by introducing @RetryableException annotation, CUSTOMER_RETRY_ERROR constant, and conditional retry logic in ProviderInvoker and FailoverCluster.
Out of Scope Changes check ✅ Passed All changes are directly aligned with the stated objective of supporting custom retryable exceptions; no out-of-scope modifications detected.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6916e2e and d08a1eb.

📒 Files selected for processing (1)
  • core/api/src/main/java/com/alipay/sofa/rpc/filter/ProviderInvoker.java (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/api/src/main/java/com/alipay/sofa/rpc/filter/ProviderInvoker.java (1)
core/exception/src/main/java/com/alipay/sofa/rpc/core/exception/RpcErrorType.java (1)
  • RpcErrorType (24-115)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build
  • GitHub Check: sca
🔇 Additional comments (1)
core/api/src/main/java/com/alipay/sofa/rpc/filter/ProviderInvoker.java (1)

21-21: LGTM!

The import is correctly added to support the new retryable exception detection feature.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sofastack-cla sofastack-cla bot added size/M and removed size/S labels Sep 7, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (2)
core/exception/src/main/java/com/alipay/sofa/rpc/core/exception/RpcErrorType.java (1)

111-114: Rename constant and add a dedicated 300-range section header.

“CUSTOMER” reads as end-user; here it’s “custom.” Also keep sectioning consistent with 100–199/200–299 blocks.

Apply:

-    /**
-     * 用户自定义可重试异常
-     */
-    public static final int CUSTOMER_RETRY_ERROR = 300;
+    // ==== 应用/业务可重试异常码 300-399 ======
+    /**
+     * 用户自定义可重试异常
+     */
+    public static final int CUSTOM_RETRY_ERROR      = 300;
core-impl/client/src/main/java/com/alipay/sofa/rpc/client/FailoverCluster.java (1)

70-78: Use the renamed error constant; logic otherwise looks good.

Keeps retries bounded and preserves existing logging behavior.

Apply:

-                    if (appResponse instanceof SofaRpcException) {
-                        if (((SofaRpcException) appResponse).getErrorType() == RpcErrorType.CUSTOMER_RETRY_ERROR) {
+                    if (appResponse instanceof SofaRpcException) {
+                        if (((SofaRpcException) appResponse).getErrorType() == RpcErrorType.CUSTOM_RETRY_ERROR) {
                             time++;
                             throwable = (SofaRpcException) appResponse;
                             continue;
                         }
                     }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2a11a24 and bff6d8d.

📒 Files selected for processing (4)
  • core-impl/client/src/main/java/com/alipay/sofa/rpc/client/FailoverCluster.java (1 hunks)
  • core/api/src/main/java/com/alipay/sofa/rpc/filter/ProviderInvoker.java (2 hunks)
  • core/common/src/main/java/com/alipay/sofa/rpc/common/annotation/RetryableException.java (1 hunks)
  • core/exception/src/main/java/com/alipay/sofa/rpc/core/exception/RpcErrorType.java (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: build
core/exception/src/main/java/com/alipay/sofa/rpc/core/exception/RpcErrorType.java

[error] 1-1: Uncommitted changes detected. Please commit core/exception/src/main/java/com/alipay/sofa/rpc/core/exception/RpcErrorType.java before continuing.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: sca
🔇 Additional comments (1)
core/exception/src/main/java/com/alipay/sofa/rpc/core/exception/RpcErrorType.java (1)

111-114: Fix CI: commit this file.

CI reports “Uncommitted changes detected” for this path. Ensure the modified file is added and committed to the PR.

@EvenLjj EvenLjj added the later This will be worked on in later version label Oct 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla:yes CLA is ok later This will be worked on in later version size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support custom exception retry

2 participants