Skip to content

Log warning in rest client response handler #2338

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.

Sign up for GitHub

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

Closed
dreamer-89 wants to merge 1 commit into from
Closed

Log warning in rest client response handler #2338

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions client/rest/src/main/java/org/opensearch/client/RestClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Copy link
Member

@andrross andrross Mar 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@dblock dblock Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @andrross here, wdyt @dreamer-89?

Copy link
Member Author

@dreamer-89 dreamer-89 Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

  1. Inside parent class OpenSearchRestTestCase
  2. 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 ?

Copy link
Member

@andrross andrross Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@dreamer-89 dreamer-89 Mar 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @andrross for the feedback. The initial failing test seems to be fixed now #2334. I will revive this PR, if I found similar failures as you suggested.

throw new WarningFailureException(response);
}
return new ResponseOrResponseException(response);
Expand Down