Log warning in rest client response handler #2338
Conversation
|
Can one of the admins verify this patch? |
|
❌ Gradle Check failure b4111570b476925c1521a2d9b5cf10a96f7f99a3 |
|
start gradle check |
|
❌ Gradle Check failure b4111570b476925c1521a2d9b5cf10a96f7f99a3 |
|
Ignore me... still failing w/ fix |
|
I think we need to rebase this PR to pickup that change and run the check again. @dreamer-89 Care to try? |
|
Let's wait until #2342 is merged... |
|
Will wait for #2342 to merge before rebasing |
ready for rebase |
…ures Signed-off-by: Suraj Singh <surajrider@gmail.com>
| @@ -343,6 +343,7 @@ private ResponseOrResponseException convertResponse(InternalRequest request, Nod | |||
| if (isSuccessfulResponse(statusCode) || request.ignoreErrorCodes.contains(response.getStatusLine().getStatusCode())) { | |||
| onResponse(node); | |||
| if (request.warningsHandler.warningsShouldFailRequest(response.getWarnings())) { | |||
| logger.warn(Arrays.toString(response.getWarnings().toArray())); | |||
There was a problem hiding this comment.
log-and-throw is generally an anti-pattern. We really can't know whether logging is the right thing to do in all contexts for a general purpose client like this, so it is usually better to let whatever handles the exception make the logging decision. In the context of the tests it is definitely a good idea to log these details. Is it possible to handle this exception in the test runner somewhere and log it there?
There was a problem hiding this comment.
I agree with @andrross here, wdyt @dreamer-89?
There was a problem hiding this comment.
Thanks @andrross @dblock for the feedback. Yes, I also agree with @andrross here.
The test recoverReplica directly calls the rest client via parent class OpenSearchRestTestCase method. So, there are couple of options for adding log statement;
- Inside parent class
OpenSearchRestTestCase - Individual test cases.
#1 seems better option as its minor change and covers all cases.
I think since issue the original issue is fixed in #2334; we can hold onto this change for now ?
There was a problem hiding this comment.
Logging these warnings in the parent seems like a good idea to me, unless it creates a huge amount of unnecessary log statements in the test output. If it is a quick and easy change I'd say go ahead and do it. If not, then we can hold off until it is needed in the future.
| @@ -343,6 +343,7 @@ private ResponseOrResponseException convertResponse(InternalRequest request, Nod | |||
| if (isSuccessfulResponse(statusCode) || request.ignoreErrorCodes.contains(response.getStatusLine().getStatusCode())) { | |||
| onResponse(node); | |||
| if (request.warningsHandler.warningsShouldFailRequest(response.getWarnings())) { | |||
| logger.warn(Arrays.toString(response.getWarnings().toArray())); | |||
There was a problem hiding this comment.
I agree with @andrross here, wdyt @dreamer-89?
|
@dreamer-89 Can we close out this PR for now? |
Sure @andrross . Closing this PR as it is not necessary now. The original issue which we wanted to debug is already fixed in #2334 |
Signed-off-by: Suraj Singh surajrider@gmail.com
Description
Log warnings in rest client response handler causing test failures. This change is to get insights on warnings which is causing the test failures in gradle check.
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.