[Feature/extensions] Pass full RestResponse to user from Extension #4356
Conversation
Signed-off-by: Daniel Widdis <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## feature/extensions #4356 +/- ##
========================================================
- Coverage 70.61% 70.59% -0.02%
- Complexity 57271 57301 +30
========================================================
Files 4633 4633
Lines 275817 275850 +33
Branches 40337 40337
========================================================
- Hits 194763 194738 -25
- Misses 64722 64886 +164
+ Partials 16332 16226 -106
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
dbwiddis
requested review from
joshpalis,
owaiskazi19,
ryanbogan and
saratvemulapalli
| assertEquals(2, fooList.size()); | ||
| assertTrue(fooList.containsAll(headerValueList)); | ||
|
|
||
| try (BytesStreamOutput out = new BytesStreamOutput()) { |
There was a problem hiding this comment.
This is great. Now we are testing input/output byte stream as well. Can we create an issue to test the other request/response as well we have for extensibility?
There was a problem hiding this comment.
It's a great thing that we're testing it, as when my test initially failed I discovered write(byte[]) is different than writeByteArray(byte[]). :-)
Yes, we should test all the other request/response code similarly. There's a small list of other fixes I'd like (compiler warnings, etc.) that I'll put in an issue later this week.
| final StringBuilder responseBuilder = new StringBuilder(); | ||
| // Initialize response. Values will be changed in the handler. | ||
| final RestExecuteOnExtensionResponse restExecuteOnExtensionResponse = new RestExecuteOnExtensionResponse( | ||
| RestStatus.ACCEPTED, |
There was a problem hiding this comment.
Do we need to keep the initial status as ACCEPTED? Can we initialize with NOT_FOUND or something in the range of 4xx?
There was a problem hiding this comment.
Good question. I scanned the list and thought this was a reasonable "current state" but on looking at the docs it seems to imply a "final" async call, so it's probably not right. Not sure if 404 is better than 500 as a default otherwise.
I think it's a moot point, though, as it's always overwritten. With the countdown latch it will only return from the two methods that count down, or from a timeout. So we could set it null and it probably wouldn't matter.
- On success it's overwritten by the response sent by the extension's handler.
- On exception it's a 500.
- On timeout waiting for the response it's a 408.
Leaning toward 500 at this point, will see what other reviewers have to say.
There was a problem hiding this comment.
Switching to 500 which will save me having to overwrite it on the exception.
There was a problem hiding this comment.
Okay. So I remembered, recently I worked on a bug for OpenSearch which asked the API response to be changed from 500 to 4xx because it should be an exception rather server error. I'm still thinking around should we keep it 500 or something on 4xx line. Let's see what other reviewers have to say.
There was a problem hiding this comment.
@saratvemulapalli any thoughts here?
There was a problem hiding this comment.
500 sounds perfect, its designed for server errors.
Signed-off-by: Daniel Widdis <[email protected]>
Gradle Check (Jenkins) Run Completed with:
|
| restExecuteOnExtensionResponse.setStatus(response.getStatus()); | ||
| restExecuteOnExtensionResponse.setContentType(response.getContentType()); | ||
| restExecuteOnExtensionResponse.setContent(response.getContent()); | ||
| restExecuteOnExtensionResponse.setHeaders(response.getHeaders()); | ||
| inProgressLatch.countDown(); |
There was a problem hiding this comment.
We have to figure out a way to remove latches. I think OpenSearch does it via Listeners.
Companion PR: opensearch-project/opensearch-sdk-java#114
Description
In the initial SDK implementation, I simply returned a
Stringfrom the Extension's REST handler to the user.This PR updates the (SDK API) return value to a
RestResponseto match what a user would have received from OpenSearch itself or a plugin, including the HTTP Status Code, and the ability to return formats other than text.As a bonus, I figured out how to test the input and output streams in the Request/Response classes and have improved those tests as well.
Issues Resolved
Fixes SDK #112
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.