-
Notifications
You must be signed in to change notification settings - Fork 133
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.
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
Support for better handling error from ES search #422
Conversation
👍 No lint errors found. |
2577321
to
8c291ac
Compare
👍 No lint errors found. |
UNIT TESTS HAVE PASSED... Good To Merge |
Codecov Report
@@ Coverage Diff @@
## develop #422 +/- ##
===========================================
- Coverage 41.03% 41.02% -0.01%
===========================================
Files 91 91
Lines 9436 9433 -3
===========================================
- Hits 3872 3870 -2
+ Misses 5564 5563 -1
Continue to review full report at Codecov.
|
8c291ac
to
b93a007
Compare
👍 No lint errors found. |
👍 No lint errors found. |
UNIT TESTS HAVE PASSED... Good To Merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current set of changes are okay but they don't address the wrong response code and don't log the failing es queries
For hits miss raised Rwquest Exceptions
👍 No lint errors found. |
UNIT TESTS HAVE PASSED... Good To Merge |
ner_v1/api.py
Outdated
except TypeError as err: | ||
ner_logger.exception(f"Error in text_synonym for: {request.path}, error: {err}") | ||
return HttpResponse(status=500) | ||
except KeyError as err: | ||
ner_logger.exception(f"Error in text_synonym for: {request.path}, error: {err}") | ||
return HttpResponse(status=500) | ||
except es_exceptions.ConnectionTimeout as err: | ||
ner_logger.exception(f"Error in text_synonym for: {request.path}, error: {err}") | ||
return HttpResponse(status=500) | ||
except es_exceptions.ConnectionError as err: | ||
ner_logger.exception(f"Error in text_synonym for: {request.path}, error: {err}") | ||
return HttpResponse(status=500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, considering we are doing the exact same thing we can combine the blocks
except (TypeError, KeyError, ...) as err
except TypeError as e: | ||
ner_logger.exception('Exception for city: %s ' % e) | ||
return HttpResponse(status=500) | ||
except KeyError as e: | ||
ner_logger.exception('Exception for text_synonym: %s ' % e) | ||
return HttpResponse(status=500) | ||
except es_exceptions.ConnectionTimeout as e: | ||
ner_logger.exception('Exception for text_synonym: %s ' % e) | ||
return HttpResponse(status=500) | ||
except es_exceptions.ConnectionError as e: | ||
ner_logger.exception('Exception for text_synonym: %s ' % e) | ||
return HttpResponse(status=500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some more suggestions, please verify it will be easy to grep the Datastore exception cases along with the queries and the responses from logs because without structured logging multiline logs (like tracebacks) from multiple parallel workers can get entangled and hard to decipher
…ute to catch at root level - Fix error messages to make it more readble
👍 No lint errors found. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM 👍
Please manually test the new ES exception cases if they are being raised and logged in a useable format
Can delete the empty datastore_exceptions.py
file at the root, but that's not a blocker
UNIT TESTS HAVE PASSED... Good To Merge |
JIRA Ticket Number
JIRA TICKET: ML-2249
Description of change
Adding support for handling exceptions raised from ES for:
Checklist (OPTIONAL):