feature: Add comprehensive HTTP exception classification for retry logic#330
feature: Add comprehensive HTTP exception classification for retry logic#330
Conversation
Import robust exception handling from airframe-http including: - HttpExceptionClassifier for nuanced HTTP response classification (5xx retryable, selective 4xx: 408, 410, 429, 499) - Platform-specific exception classifiers via HttpCompat (JVM: SSL/connection exceptions; JS/Native: stubs) - HttpMaxRetryException with concrete response/status access - Expanded HttpStatus.isRetryable to include 410 and 499 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @xerial, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the HTTP client's resilience by introducing a sophisticated and comprehensive exception classification system for retry logic. It centralizes the decision-making process for whether an HTTP response or an underlying exception warrants a retry, adapting behavior based on HTTP status codes and various exception types. The changes also ensure platform-specific compatibility for JVM, Scala.js, and Scala Native, and provide a more informative exception when maximum retry attempts are exhausted, ultimately leading to a more robust and fault-tolerant HTTP communication layer. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive and robust exception classification mechanism for HTTP client retry logic, largely ported from airframe-http. The changes are well-structured, with a central HttpExceptionClassifier, platform-specific implementations via HttpCompat, and a new HttpMaxRetryException for better error handling. The code is clean, and the addition of extensive tests is commendable. My review found one minor issue regarding redundant code in the JVM-specific exception classifier. Overall, this is an excellent improvement to the library's resilience.
| e match | ||
| case _: BindException => | ||
| retryableFailure(e) | ||
| case _: ConnectException => | ||
| retryableFailure(e) | ||
| case _: NoRouteToHostException => | ||
| retryableFailure(e) | ||
| case _: PortUnreachableException => | ||
| retryableFailure(e) | ||
| case se if se.getMessage == "Socket closed" => | ||
| retryableFailure(e) | ||
| case _ => | ||
| nonRetryableFailure(e) |
There was a problem hiding this comment.
The case _: ConnectException within this match block is redundant. ConnectException is already handled by a more specific case on line 54. Since java.net.ConnectException extends java.net.SocketException, any ConnectException will be matched by case e: ConnectException before reaching this case e: SocketException block, making the inner case for ConnectException unreachable dead code. Removing it will improve code clarity and maintainability.
e match
case _: BindException =>
retryableFailure(e)
case _: NoRouteToHostException =>
retryableFailure(e)
case _: PortUnreachableException =>
retryableFailure(e)
case se if se.getMessage == "Socket closed" =>
retryableFailure(e)
case _ =>
nonRetryableFailure(e)…atch ConnectException extends SocketException and is already handled by the outer case before reaching the SocketException block, making the inner case unreachable dead code. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Key Changes
Test plan
🤖 Generated with Claude Code