Fixed client processing time #462
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: saimedhi <saimedhi@amazon.com>
|
@IanHoang, @gkamat Please take a look. This PR fixes #450 (comment) |
|
sample benchmark result when there is an error in force-merge |
|
@saimedhi For future reference, please provide clearer context in PR description for others who might not have full-context on what this PR fixes. Could you provide the following as well:
|
summary results(created failure in forcemerge calls by making changes in opensearch-py client used in benchmarking). |
|
benchmark-metrics-* document |
Do you have a sample document for any force-merges? You can filter the dashboards to look for operation-type = force-merge |
|
@IanHoang, I believe you're the best person to help ensure all possible failure scenarios are thoroughly tested. Could you please review and test each case for failure? Let me know if any changes are needed. Thank you! |
gkamat
requested changes
Feb 13, 2024
Comment on lines
1678
to
1684
| total_ops_unit = "ops" | ||
| request_meta_data = {"success": True} | ||
| except opensearchpy.TransportError as e: | ||
| request_context_holder.on_client_request_end() | ||
| # we *specifically* want to distinguish connection refused (a node died?) from connection timeouts | ||
| # pylint: disable=unidiomatic-typecheck | ||
| if type(e) is opensearchpy.ConnectionError: |
There was a problem hiding this comment.
Generally, it is better to catch any exceptions raised by an API call at the point where the call is made, rather than up the call stack. That way, the caller can tailor the exceptions handled precisely, for instance, a search call may raise a different set of exceptions as compared to a force-merge. Otherwise, someone looking at the higher level call needs to dig down possible flows of control to identify exceptions that might get raised.
The current implementation of execute_single() is legacy in that it assumes it is tied to the ES/OS API. In the future, we will support other clients, that might raise other exceptions. Therefore, the strategy noted above would be preferable to the one implemented in this PR.
BTW, the execute_singe() routine should indeed capture exceptions, but they should be more general, just to handle the eventuality that lower-level modules have a bug and missed catching something they should.
In summary, the change above is functionally correct, but it is recommended that exceptions be caught at the point of call.
There was a problem hiding this comment.
I understand your point, @gkamat. I'm not introducing new exception handling; I'm simply addressing the issue mentioned in #450 (comment).
And I believe the error handling as said here is unrelated to my PR. Lets raise a separate issue for it if required. Thank you.
There was a problem hiding this comment.
The proposed change requires exception handling for the client time computation functionality, else there is a KeyError that gets raised. This should be caught at the point of the call, rather than up the stack.
The overall change does not need to be reverted; just exception handling for the current functionality needs to be added at each API call, rather than relying on the existing checks. Ideally, the checks in execute-single might be moved down, but that is not a prerequisite.
IanHoang
approved these changes
Feb 14, 2024
|
As discussed, I've opened an issue to refine the error handling strategy in the execute_single function. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixed client processing time
Issues Resolved
Fixed an issue raised with client processing time addition.
#450 (comment)
Testing
[Describe how this change was tested]
I tested it in most of the cases where OpenSearch call fails.
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.