Refactor response handlers to improve error handling and streamline mid-stream error processing #128923
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…id-stream error processing
elasticsearchmachine
added
v9.1.0
external-contributor
6 tasks
|
|
||
| protected final String requestType; | ||
| protected final ResponseParser parseFunction; | ||
| private final Function<HttpResult, ErrorResponse> errorParseFunction; | ||
| private final boolean canHandleStreamingResponses; | ||
|
|
||
| public BaseResponseHandler(String requestType, ResponseParser parseFunction, Function<HttpResult, ErrorResponse> errorParseFunction) { |
There was a problem hiding this comment.
This constructor is not used anywhere but children classes, making it protected for that reason.
| this(requestType, parseFunction, errorParseFunction, false); | ||
| } | ||
|
|
||
| public BaseResponseHandler( | ||
| protected BaseResponseHandler( |
There was a problem hiding this comment.
This constructor is not used anywhere but children classes, making it protected for that reason.
| var errorEntityMsg = errorParseFunction.apply(result); | ||
| return buildError(message, request, result, errorEntityMsg); | ||
| var errorResponse = errorParseFunction.apply(result); | ||
| return buildError(message, request, result, errorResponse); |
There was a problem hiding this comment.
errorParseFunction creates ErrorResponse instance, not a message. errorEntityMsg variable naming doesn't really make sense here. Fixed.
| } | ||
|
|
||
| protected Exception buildError(String message, Request request, HttpResult result, ErrorResponse errorResponse) { | ||
| var responseStatusCode = result.response().getStatusLine().getStatusCode(); | ||
| return new ElasticsearchStatusException( | ||
| errorMessage(message, request, result, errorResponse, responseStatusCode), |
There was a problem hiding this comment.
Removed unused parameter
| Exception e, | ||
| Class<? extends ErrorResponse> errorResponseClass | ||
| ) { | ||
| // Extract the error response from the message using the provided method |
There was a problem hiding this comment.
Let me know if commenting here is excessive.
| * @return a string representing the error type | ||
| */ | ||
| protected static String createErrorType(ErrorResponse errorResponse) { | ||
| return errorResponse != null ? errorResponse.getClass().getSimpleName() : "unknown"; |
There was a problem hiding this comment.
Moved this from several handlers so it can be used directly.
jonathan-buttner
added
:ml
|
Pinging @elastic/ml-core (Team:ML) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This refactoring is initiated by @jonathan-buttner 's comment and moves provider-independent, repeated logic to BaseResponseHandler, allowing all handlers dealing with streaming errors to reuse it. The goal is to reduce duplication and standardize error handling across providers.
Design Trade-offs and Considerations
1. Class Hierarchy Limitations
Currently, all streaming handlers inherit from non-streaming handlers. As a result, shared methods related to streaming error handling become accessible in contexts where they are not applicable (i.e., non-streaming handlers).
This is a compromise to avoid duplicating logic, but it exposes potentially misleading APIs in non-streaming classes. A more robust solution would involve a cleaner separation in the class hierarchy, e.g., extracting a common intermediate base class or using composition.
2. Enforcing Overrides with UnsupportedOperationException
For provider-specific methods that must be overridden, I've added UnsupportedOperationException as a safeguard.
This ensures that if a required override is missing and the method is called, it fails fast. However, this is a runtime check. Ideally, we'd enforce such overrides at compile time, possibly using abstract methods or interfaces. I’m open to suggestions here if we want stricter enforcement. In current implementation it is handled in some places using
assert request.isStreaming().3. Error Handling Scenarios
There are two main error-handling scenarios:
4. Provider-Specific Handler Overview
5. Use of instanceof and Casting
Currently, ErrorResponse types are checked via instanceof, and casting is done manually. While an alternative could be using reflection or a more abstracted pattern, I’m deliberately avoiding reflection due to it being risky impact.
A more structured approach (e.g., visitor pattern or dedicated handler interfaces per provider) would require a broader refactor, which may be worth considering in the future.
6. Regarding the Mistral PR Comment
As mentioned in the Mistral PR discussion, lifting instanceof checks higher in the hierarchy would impact the flow for other providers. This refactoring keeps the current behavior intact while still avoiding duplication by moving shared logic upward in the hierarchy.
Final Thoughts
This refactoring improves maintainability by reducing code duplication and providing a flexible way to build UnifiedChatCompletionException based on provider-specific logic. However, it introduces some technical debt due to the existing class hierarchy and runtime enforcement of required overrides.
I’d appreciate any feedback on whether we should consider a more structural redesign (e.g., class hierarchy split or polymorphic error handling). Didn't want to go too far into changing architecture because it wasn't requested in initial comment.
gradle check?