Skip to content

Commit de4245c

Browse files
jonathan-buttnerjonathan-buttner
authored
Removing error object check (elastic#132502)
1 parent e223df1 commit de4245c

16 files changed

+35
-130
lines changed

‎x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/external/http/retry/BaseResponseHandler.java‎

Lines changed: 1 addition & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -76,38 +76,13 @@ public String getRequestType() {
7676
}
7777

7878
@Override
79-
public void validateResponse(
80-
ThrottlerManager throttlerManager,
81-
Logger logger,
82-
Request request,
83-
HttpResult result,
84-
boolean checkForErrorObject
85-
) {
79+
public void validateResponse(ThrottlerManager throttlerManager, Logger logger, Request request, HttpResult result) {
8680
checkForFailureStatusCode(request, result);
8781
checkForEmptyBody(throttlerManager, logger, request, result);
88-
89-
if (checkForErrorObject) {
90-
// When the response is streamed the status code could be 200 but the error object will be set
91-
// so we need to check for that specifically
92-
checkForErrorObject(request, result);
93-
}
9482
}
9583

9684
protected abstract void checkForFailureStatusCode(Request request, HttpResult result);
9785

98-
protected void checkForErrorObject(Request request, HttpResult result) {
99-
var errorEntity = errorParseFunction.apply(result);
100-
101-
if (errorEntity.errorStructureFound()) {
102-
// We don't really know what happened because the status code was 200 so we'll return a failure and let the
103-
// client retry if necessary
104-
// If we did want to retry here, we'll need to determine if this was a streaming request, if it was
105-
// we shouldn't retry because that would replay the entire streaming request and the client would get
106-
// duplicate chunks back
107-
throw new RetryException(false, buildError(SERVER_ERROR_OBJECT, request, result, errorEntity));
108-
}
109-
}
110-
11186
protected Exception buildError(String message, Request request, HttpResult result) {
11287
var errorEntityMsg = errorParseFunction.apply(result);
11388
return buildError(message, request, result, errorEntityMsg);

‎x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/external/http/retry/ChatCompletionErrorResponseHandler.java‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -28,19 +28,6 @@ public ChatCompletionErrorResponseHandler(UnifiedChatCompletionErrorParser error
2828
this.unifiedChatCompletionErrorParser = Objects.requireNonNull(errorParser);
2929
}
3030

31-
public void checkForErrorObject(Request request, HttpResult result) {
32-
var errorEntity = unifiedChatCompletionErrorParser.parse(result);
33-
34-
if (errorEntity.errorStructureFound()) {
35-
// We don't really know what happened because the status code was 200 so we'll return a failure and let the
36-
// client retry if necessary
37-
// If we did want to retry here, we'll need to determine if this was a streaming request, if it was
38-
// we shouldn't retry because that would replay the entire streaming request and the client would get
39-
// duplicate chunks back
40-
throw new RetryException(false, buildChatCompletionErrorInternal(SERVER_ERROR_OBJECT, request, result, errorEntity));
41-
}
42-
}
43-
4431
public UnifiedChatCompletionException buildChatCompletionError(String message, Request request, HttpResult result) {
4532
var errorResponse = unifiedChatCompletionErrorParser.parse(result);
4633
return buildChatCompletionErrorInternal(message, request, result, errorResponse);

‎x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/external/http/retry/ResponseHandler.java‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,9 @@ public interface ResponseHandler {
2929
* @param logger the logger to use for logging
3030
* @param request the original request
3131
* @param result the response from the server
32-
* @param checkForErrorObject if true, the validation function should check for the presence of an error object even if the status code
33-
* indicates a success
3432
* @throws RetryException if the response is invalid
3533
*/
36-
void validateResponse(ThrottlerManager throttlerManager, Logger logger, Request request, HttpResult result, boolean checkForErrorObject)
37-
throws RetryException;
34+
void validateResponse(ThrottlerManager throttlerManager, Logger logger, Request request, HttpResult result) throws RetryException;
3835

3936
/**
4037
* A method for parsing the response from the server.

‎x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/external/http/retry/RetryingHttpSender.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ public void tryAction(ActionListener<InferenceServiceResults> listener) {
121121
} else {
122122
r.readFullResponse(l.delegateFailureAndWrap((ll, httpResult) -> {
123123
try {
124-
responseHandler.validateResponse(throttlerManager, logger, request, httpResult, true);
124+
responseHandler.validateResponse(throttlerManager, logger, request, httpResult);
125125
InferenceServiceResults inferenceResults = responseHandler.parseResult(request, httpResult);
126126
ll.onResponse(inferenceResults);
127127
} catch (Exception e) {
@@ -134,7 +134,7 @@ public void tryAction(ActionListener<InferenceServiceResults> listener) {
134134
} else {
135135
httpClient.send(request.createHttpRequest(), context, retryableListener.delegateFailure((l, r) -> {
136136
try {
137-
responseHandler.validateResponse(throttlerManager, logger, request, r, false);
137+
responseHandler.validateResponse(throttlerManager, logger, request, r);
138138
InferenceServiceResults inferenceResults = responseHandler.parseResult(request, r);
139139

140140
l.onResponse(inferenceResults);

‎x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/amazonbedrock/response/AmazonBedrockResponseHandler.java‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,8 @@ public boolean canHandleStreamingResponses() {
2222
}
2323

2424
@Override
25-
public final void validateResponse(
26-
ThrottlerManager throttlerManager,
27-
Logger logger,
28-
Request request,
29-
HttpResult result,
30-
boolean checkForErrorObject
31-
) throws RetryException {
25+
public final void validateResponse(ThrottlerManager throttlerManager, Logger logger, Request request, HttpResult result)
26+
throws RetryException {
3227
// do nothing as the AWS SDK will take care of validation for us
3328
}
3429
}

‎x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/azureopenai/response/AzureMistralOpenAiExternalResponseHandler.java‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -63,13 +63,8 @@ public AzureMistralOpenAiExternalResponseHandler(
6363
}
6464

6565
@Override
66-
public void validateResponse(
67-
ThrottlerManager throttlerManager,
68-
Logger logger,
69-
Request request,
70-
HttpResult result,
71-
boolean checkForErrorObject
72-
) throws RetryException {
66+
public void validateResponse(ThrottlerManager throttlerManager, Logger logger, Request request, HttpResult result)
67+
throws RetryException {
7368
checkForFailureStatusCode(request, result);
7469
checkForEmptyBody(throttlerManager, logger, request, result);
7570
}

‎x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/services/googlevertexai/GoogleVertexAiUnifiedChatCompletionResponseHandler.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,11 +65,6 @@ protected UnifiedChatCompletionException buildError(String message, Request requ
6565
return chatCompletionErrorResponseHandler.buildChatCompletionError(message, request, result);
6666
}
6767

68-
@Override
69-
protected void checkForErrorObject(Request request, HttpResult result) {
70-
chatCompletionErrorResponseHandler.checkForErrorObject(request, result);
71-
}
72-
7368
private static class GoogleVertexAiErrorParser implements UnifiedChatCompletionErrorParser {
7469

7570
@Override

‎x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/external/http/retry/AlwaysRetryingResponseHandler.java‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,8 @@ public AlwaysRetryingResponseHandler(
3636
}
3737

3838
@Override
39-
public void validateResponse(
40-
ThrottlerManager throttlerManager,
41-
Logger logger,
42-
Request request,
43-
HttpResult result,
44-
boolean checkForErrorObject
45-
) throws RetryException {
39+
public void validateResponse(ThrottlerManager throttlerManager, Logger logger, Request request, HttpResult result)
40+
throws RetryException {
4641
try {
4742
checkForFailureStatusCode(throttlerManager, logger, request, result);
4843
checkForEmptyBody(throttlerManager, logger, request, result);

‎x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/external/http/retry/BaseResponseHandlerTests.java‎

Lines changed: 14 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -59,12 +59,11 @@ public void testValidateResponse_DoesNotThrowAnExceptionWhenStatus200_AndNoError
5959
mock(ThrottlerManager.class),
6060
mock(Logger.class),
6161
request,
62-
new HttpResult(response, responseJson.getBytes(StandardCharsets.UTF_8)),
63-
true
62+
new HttpResult(response, responseJson.getBytes(StandardCharsets.UTF_8))
6463
);
6564
}
6665

67-
public void testValidateResponse_ThrowsErrorWhenMalformedErrorObjectExists() {
66+
public void testValidateResponse_DoesNotThrowError_WhenStatus200_AndMalformedErrorObject() {
6867
var handler = getBaseResponseHandler();
6968

7069
String responseJson = """
@@ -80,25 +79,15 @@ public void testValidateResponse_ThrowsErrorWhenMalformedErrorObjectExists() {
8079
var request = mock(Request.class);
8180
when(request.getInferenceEntityId()).thenReturn("abc");
8281

83-
var exception = expectThrows(
84-
RetryException.class,
85-
() -> handler.validateResponse(
86-
mock(ThrottlerManager.class),
87-
mock(Logger.class),
88-
request,
89-
new HttpResult(response, responseJson.getBytes(StandardCharsets.UTF_8)),
90-
true
91-
)
92-
);
93-
94-
assertFalse(exception.shouldRetry());
95-
assertThat(
96-
exception.getCause().getMessage(),
97-
is("Received an error response for request from inference entity id [abc] status [200]")
82+
handler.validateResponse(
83+
mock(ThrottlerManager.class),
84+
mock(Logger.class),
85+
request,
86+
new HttpResult(response, responseJson.getBytes(StandardCharsets.UTF_8))
9887
);
9988
}
10089

101-
public void testValidateResponse_ThrowsErrorWhenWellFormedErrorObjectExists() {
90+
public void testValidateResponse_DoesNotThrow_WhenStatus200_AndWellFormedErrorObjectExists() {
10291
var handler = getBaseResponseHandler();
10392

10493
String responseJson = """
@@ -115,21 +104,11 @@ public void testValidateResponse_ThrowsErrorWhenWellFormedErrorObjectExists() {
115104
var request = mock(Request.class);
116105
when(request.getInferenceEntityId()).thenReturn("abc");
117106

118-
var exception = expectThrows(
119-
RetryException.class,
120-
() -> handler.validateResponse(
121-
mock(ThrottlerManager.class),
122-
mock(Logger.class),
123-
request,
124-
new HttpResult(response, responseJson.getBytes(StandardCharsets.UTF_8)),
125-
true
126-
)
127-
);
128-
129-
assertFalse(exception.shouldRetry());
130-
assertThat(
131-
exception.getCause().getMessage(),
132-
is("Received an error response for request from inference entity id [abc] status [200]. Error message: [a message]")
107+
handler.validateResponse(
108+
mock(ThrottlerManager.class),
109+
mock(Logger.class),
110+
request,
111+
new HttpResult(response, responseJson.getBytes(StandardCharsets.UTF_8))
133112
);
134113
}
135114

@@ -154,8 +133,7 @@ public void testValidateResponse_DoesNot_ThrowErrorWhenWellFormedErrorObjectExis
154133
mock(ThrottlerManager.class),
155134
mock(Logger.class),
156135
request,
157-
new HttpResult(response, responseJson.getBytes(StandardCharsets.UTF_8)),
158-
false
136+
new HttpResult(response, responseJson.getBytes(StandardCharsets.UTF_8))
159137
);
160138
}
161139

‎x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/external/http/retry/RetryingHttpSenderTests.java‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@
4242
import static org.hamcrest.Matchers.is;
4343
import static org.hamcrest.Matchers.sameInstance;
4444
import static org.mockito.ArgumentMatchers.any;
45-
import static org.mockito.ArgumentMatchers.anyBoolean;
4645
import static org.mockito.Mockito.doAnswer;
4746
import static org.mockito.Mockito.doThrow;
4847
import static org.mockito.Mockito.mock;
@@ -77,7 +76,7 @@ public void testSend_CallsSenderAgain_AfterValidateResponseThrowsAnException() t
7776
Answer<InferenceServiceResults> answer = (invocation) -> inferenceResults;
7877

7978
var handler = mock(ResponseHandler.class);
80-
doThrow(new RetryException(true, "failed")).doNothing().when(handler).validateResponse(any(), any(), any(), any(), anyBoolean());
79+
doThrow(new RetryException(true, "failed")).doNothing().when(handler).validateResponse(any(), any(), any(), any());
8180
// Mockito.thenReturn() does not compile when returning a
8281
// bounded wild card list, thenAnswer must be used instead.
8382
when(handler.parseResult(any(Request.class), any(HttpResult.class))).thenAnswer(answer);
@@ -352,7 +351,7 @@ public void testSend_ReturnsFailure_WhenValidateResponseThrowsAnException_AfterO
352351
var handler = mock(ResponseHandler.class);
353352
doThrow(new RetryException(true, "failed")).doThrow(new IllegalStateException("failed again"))
354353
.when(handler)
355-
.validateResponse(any(), any(), any(), any(), anyBoolean());
354+
.validateResponse(any(), any(), any(), any());
356355
when(handler.parseResult(any(Request.class), any(HttpResult.class))).thenAnswer(answer);
357356

358357
var retrier = createRetrier(sender);
@@ -389,7 +388,7 @@ public void testSend_ReturnsFailure_WhenValidateResponseThrowsAnElasticsearchExc
389388
var handler = mock(ResponseHandler.class);
390389
doThrow(new RetryException(true, "failed")).doThrow(new RetryException(false, "failed again"))
391390
.when(handler)
392-
.validateResponse(any(), any(), any(), any(), anyBoolean());
391+
.validateResponse(any(), any(), any(), any());
393392
when(handler.parseResult(any(Request.class), any(HttpResult.class))).thenAnswer(answer);
394393

395394
var retrier = createRetrier(httpClient);
@@ -702,13 +701,8 @@ private ResponseHandler createRetryingResponseHandler() {
702701
// testing failed requests
703702
return new ResponseHandler() {
704703
@Override
705-
public void validateResponse(
706-
ThrottlerManager throttlerManager,
707-
Logger logger,
708-
Request request,
709-
HttpResult result,
710-
boolean checkForErrorObject
711-
) throws RetryException {
704+
public void validateResponse(ThrottlerManager throttlerManager, Logger logger, Request request, HttpResult result)
705+
throws RetryException {
712706
throw new RetryException(true, new IOException("response handler validate failed as designed"));
713707
}
714708

0 commit comments

Comments
 (0)