-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[ML] Exposing the original request to the error response handler logic #127859
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| import org.elasticsearch.xcontent.XContentType; | ||
| import org.elasticsearch.xpack.inference.external.http.HttpResult; | ||
| import org.elasticsearch.xpack.inference.external.http.retry.ErrorResponse; | ||
| import org.elasticsearch.xpack.inference.external.request.Request; | ||
|
|
||
| import java.util.Map; | ||
| import java.util.Objects; | ||
|
|
@@ -68,4 +69,8 @@ public static ErrorResponse fromResponse(HttpResult response, String defaultMess | |
| public static ErrorResponse fromResponse(HttpResult response) { | ||
| return fromResponse(response, ""); | ||
| } | ||
|
|
||
| public static ErrorResponse fromResponse(Request request, HttpResult response) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit unclear why we drop the request here? Is this where we would make a follow-up change to utilize the request? |
||
| return fromResponse(response); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
| import org.elasticsearch.xcontent.XContentType; | ||
| import org.elasticsearch.xpack.inference.external.http.HttpResult; | ||
| import org.elasticsearch.xpack.inference.external.http.retry.ErrorResponse; | ||
| import org.elasticsearch.xpack.inference.external.request.Request; | ||
|
|
||
| public class AlibabaCloudSearchErrorResponseEntity extends ErrorResponse { | ||
| private static final Logger logger = LogManager.getLogger(AlibabaCloudSearchErrorResponseEntity.class); | ||
|
|
@@ -23,6 +24,10 @@ private AlibabaCloudSearchErrorResponseEntity(String errorMessage) { | |
| super(errorMessage); | ||
| } | ||
|
|
||
| public static ErrorResponse fromResponse(Request request, HttpResult response) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add this to the |
||
| return fromResponse(response); | ||
| } | ||
|
|
||
| /** | ||
| * An example error response for invalid auth would look like | ||
| * <code> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to understand the context better, why do we need the inference ID as part of the error in the custom service PR? Is there a reason we need the full Request here instead of passing just the inference ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I can just pass the inference id to limit the scope 👍