Add error responses to ML Delete API documentation#11957
Add error responses to ML Delete API documentation#11957kolchfa-aws wants to merge 2 commits intomainfrom
Conversation
Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
kolchfa-aws
requested review from
AMoo-Miki,
dlvenable,
epugh,
natebower and
sumobrian
as code owners
|
Thank you for submitting your PR. The PR states are In progress (or Draft) -> Tech review -> Doc review -> Merged. Before you submit your PR for doc review, make sure the content is technically accurate. If you need help finding a tech reviewer, tag a maintainer. When you're ready for doc review, tag the assignee of this PR. The doc reviewer may push edits to the PR directly or leave comments and editorial suggestions for you to address (let us know in a comment if you have a preference). |
|
@b4sjoo Could you review this PR? |
|
@ylwu-amzn Could you review this PR? |
|
|
||
| ## Error responses | ||
|
|
||
| If you attempt to delete a memory container that doesn't exist, the API behavior may vary. Ensure that the memory container exists before attempting to delete it. |
There was a problem hiding this comment.
may vary is vague . @b4sjoo, can you share what will exactly happen?
There was a problem hiding this comment.
Looking at the code, there is a 500 error path and 404 error path. The 404 error path only occurs if the container passes the initial retrieval check but then fails during the actual deletion operation, which is less common. I think a 404 should be returned in both cases. Let me open an issue in the ml-commons repo.
There was a problem hiding this comment.
Here's the ml-commons issue opensearch-project/ml-commons#4706
There was a problem hiding this comment.
Right this is a bug. 404 is expected output here
There was a problem hiding this comment.
Thanks. Should I document the current behavior or can we put in the fix for this soon in the ml-commons repo? @ylwu-amzn Please let me know.
|
|
||
| ## Error responses | ||
|
|
||
| If you attempt to delete a context management configuration that doesn't exist, the behavior depends on your OpenSearch version and configuration. The API may return an error indicating the resource was not found. |
There was a problem hiding this comment.
"The API may return an error" — so what else might it do? Developers are left guessing:
Return a 404?
@mingshl , can you help ?
There was a problem hiding this comment.
I tested it throw exception, but it's 500 error. I will raise an issue to make it 404 error.
DELETE /_plugins/_ml/context_management/sliding_window_max_40000_tokens_managers123
{
"error": {
"root_cause": [
{
"type": "runtime_exception",
"reason": "Context management template not found: sliding_window_max_40000_tokens_managers123"
}
],
"type": "runtime_exception",
"reason": "Context management template not found: sliding_window_max_40000_tokens_managers123"
},
"status": 500
}
There was a problem hiding this comment.
There was a problem hiding this comment.
So the new response is here, @kolchfa-aws you can use it for the doc
DELETE /_plugins/_ml/context_management/sliding_window_max_40000_tokens_managers123 will be:
{
"error": {
"root_cause": [
{
"type": "status_exception",
"reason": "Context management template not found: sliding_window_max_40000_tokens_managers123"
}
],
"type": "status_exception",
"reason": "Context management template not found: sliding_window_max_40000_tokens_managers123"
},
"status": 404
}```
There was a problem hiding this comment.
Thank you, @mingshl! Added this response.
Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
4 participants
Closes #7838
Checklist
For more information on following Developer Certificate of Origin and signing off your commits, please check here.