Conversation
WalkthroughA new Spring configuration class, Changes
Sequence Diagram(s)sequenceDiagram
participant ApplicationContext
participant FeignRetryConfig
participant FeignClient
ApplicationContext->>FeignRetryConfig: Initialize configuration
FeignRetryConfig->>ApplicationContext: Register Retryer bean
FeignClient->>ApplicationContext: Retrieve Retryer bean
FeignClient->>FeignClient: Use Retryer for retry logic on failures
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/java/com/example/spot/common/config/FeignRetryConfig.java (2)
7-13: Make retry parameters configurable via properties (optional)Hard-coded periods/attempts hinder environment-specific tuning. Suggest binding via ConfigurationProperties (e.g., feign.retry.period, maxPeriod, maxAttempts) for prod vs. test flexibility.
Example outline:
// @ConfigurationProperties(prefix = "feign.retry") public record FeignRetryProps(long period, long maxPeriod, int maxAttempts) {} @Bean Retryer retryer(FeignRetryProps p) { return new Retryer.Default(p.period(), p.maxPeriod(), p.maxAttempts()); }And set values in application.yml per environment.
10-13: Caution: retries on non-idempotent requestsIf any Feign clients perform non-idempotent operations (POST/PUT without idempotency keys), retries can cause duplicate side effects on transient failures. Consider:
- Retrying only idempotent methods, or
- Adding an Idempotency-Key header via a request interceptor for those endpoints.
src/main/java/com/example/spot/common/infrastructure/feign/SafeFeignExecutor.java (1)
23-23: Comment removal is fineThe inline comment removal is a no-op. The functional issue above is independent and worth fixing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/example/spot/common/config/FeignRetryConfig.java(1 hunks)src/main/java/com/example/spot/common/infrastructure/feign/SafeFeignExecutor.java(1 hunks)
⏰ 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: build_test
🔇 Additional comments (1)
src/main/java/com/example/spot/common/config/FeignRetryConfig.java (1)
10-13: Good addition: global Feign Retryer with exponential backoffRegisters a Retryer.Default(200, 800, 3). This matches the PR goal and will apply to Feign when Spring scans this config.
| @Bean | ||
| Retryer retryer() { | ||
| return new Retryer.Default(200, 800, 3); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Retry may not trigger on 5xx/429 without a RetryableException source
Feign retries only when a RetryableException is thrown (e.g., I/O errors by the client or via a custom ErrorDecoder). If you intend to retry on 5xx/429, also provide an ErrorDecoder that maps those statuses to RetryableException or use Resilience4j/Spring Retry.
Run this to check if such wiring already exists:
🏁 Script executed:
#!/bin/bash
# Look for custom ErrorDecoder implementations
rg -n "class .* implements ErrorDecoder" -A 3
# Calls that throw RetryableException explicitly
rg -n "RetryableException" -A 2
# Per-client Feign configs that might override global beans
rg -n '@FeignClient\(' -A 5
# Other Retryer beans (potential conflicts)
rg -n "Retryer\\s+[a-zA-Z_][a-zA-Z0-9_]*\\s*\\(" -n -SLength of output: 4916
Add a custom ErrorDecoder to trigger retries on HTTP 5xx/429
Currently you’ve defined a global Retryer in FeignRetryConfig (src/main/java/com/example/spot/common/config/FeignRetryConfig.java:11), but there are no custom ErrorDecoder implementations in the codebase. By default Feign only retries when a RetryableException is thrown (typically on I/O errors), not on HTTP status codes.
What to do:
- Implement and register an
ErrorDecoderbean that maps 429 and 5xx responses toRetryableException. - You can register it globally in your config or reference it in individual
@FeignClient(configuration = …)definitions.
Example snippet:
@Bean
public ErrorDecoder feignErrorDecoder() {
return (methodKey, response) -> {
int status = response.status();
if (status == 429 || (status >= 500 && status <= 599)) {
return new RetryableException(
status,
"Retry on HTTP " + status,
response.request().httpMethod(),
null,
response.request()
);
}
return new Default().decode(methodKey, response);
};
}🤖 Prompt for AI Agents
In src/main/java/com/example/spot/common/config/FeignRetryConfig.java around
lines 10 to 13, you need to add a custom ErrorDecoder bean that triggers retries
on HTTP 429 and 5xx status codes. Implement an ErrorDecoder that checks the
response status, and if it is 429 or between 500 and 599, return a
RetryableException to enable retry logic. Register this ErrorDecoder as a bean
in the same configuration class so Feign uses it globally alongside the existing
Retryer bean.
| private static String extractMessage(FeignException e) { | ||
| return e.responseBody() | ||
| .map(body -> new String(body.array())) // byte[] → String | ||
| .map(body -> new String(body.array())) | ||
| .orElse(e.getMessage()); |
There was a problem hiding this comment.
Fix ByteBuffer decoding: array() is unsafe and ignores position/limit; specify charset
Using new String(body.array()) can throw UnsupportedOperationException for direct/read-only buffers and may include extra bytes beyond the current view. Also relies on platform default charset. Decode using position/remaining and a defined charset.
Apply this diff within extractMessage:
- private static String extractMessage(FeignException e) {
- return e.responseBody()
- .map(body -> new String(body.array()))
- .orElse(e.getMessage());
- }
+ private static String extractMessage(FeignException e) {
+ return e.responseBody()
+ .map(buf -> {
+ var readOnly = buf.asReadOnlyBuffer();
+ byte[] bytes;
+ if (readOnly.hasArray()) {
+ int offset = readOnly.arrayOffset() + readOnly.position();
+ int length = readOnly.remaining();
+ bytes = java.util.Arrays.copyOfRange(readOnly.array(), offset, offset + length);
+ } else {
+ bytes = new byte[readOnly.remaining()];
+ readOnly.get(bytes);
+ }
+ return new String(bytes, java.nio.charset.StandardCharsets.UTF_8);
+ })
+ .orElse(e.getMessage());
+ }Note: This uses java.util.Arrays and java.nio.charset.StandardCharsets; import if desired or keep FQCNs as above.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static String extractMessage(FeignException e) { | |
| return e.responseBody() | |
| .map(body -> new String(body.array())) // byte[] → String | |
| .map(body -> new String(body.array())) | |
| .orElse(e.getMessage()); | |
| private static String extractMessage(FeignException e) { | |
| return e.responseBody() | |
| .map(buf -> { | |
| var readOnly = buf.asReadOnlyBuffer(); | |
| byte[] bytes; | |
| if (readOnly.hasArray()) { | |
| int offset = readOnly.arrayOffset() + readOnly.position(); | |
| int length = readOnly.remaining(); | |
| bytes = java.util.Arrays.copyOfRange( | |
| readOnly.array(), | |
| offset, | |
| offset + length | |
| ); | |
| } else { | |
| bytes = new byte[readOnly.remaining()]; | |
| readOnly.get(bytes); | |
| } | |
| return new String(bytes, java.nio.charset.StandardCharsets.UTF_8); | |
| }) | |
| .orElse(e.getMessage()); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/com/example/spot/common/infrastructure/feign/SafeFeignExecutor.java
lines 21 to 24, the method extractMessage uses new String(body.array()) which is
unsafe for ByteBuffer as it ignores position and limit and may throw
UnsupportedOperationException for direct or read-only buffers, also it uses the
platform default charset. To fix this, decode the ByteBuffer using its position
and remaining bytes with a specified charset like StandardCharsets.UTF_8, for
example by creating a byte array of size body.remaining(), copying the bytes
from the buffer starting at its current position, and then constructing the
String with the specified charset.
1 participant
#️⃣ 연관된 이슈
🔎 작업 내용
📷 스크린샷 (선택)
💬리뷰 요구사항 (선택)
Summary by CodeRabbit
New Features
Style