Generic error catching in DB #152
Merged
Conversation
This file contains hidden or 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
lmarini
approved these changes
Oct 24, 2022
| logger.info("Successfully connected to Elasticsearch") | ||
| else: | ||
| logger.info("Can not connect to Elasticsearch") | ||
| try: |
Member
There was a problem hiding this comment.
Code looks good, but I am not sure how to test it in this case. If I turn off elasticsearch, I get an error before we get to ping.
WARNING:elastic_transport.node_pool:Node <Urllib3HttpNode(http://localhost:9200)> has failed for 11 times in a row, putting on 30 second timeout
WARNING:elastic_transport.transport:Retrying request after failure (attempt 2 of 3)
Traceback (most recent call last):
File "/Users/lmarini/.local/share/virtualenvs/clowder2-pkRZ1537/lib/python3.9/site-packages/elastic_transport/_transport.py", line 329, in perform_request
meta, raw_data = node.perform_request(
File "/Users/lmarini/.local/share/virtualenvs/clowder2-pkRZ1537/lib/python3.9/site-packages/elastic_transport/_node/_http_urllib3.py", line 199, in perform_request
raise err from None
elastic_transport.ConnectionError: Connection error caused by: NewConnectionError(<urllib3.connection.HTTPConnection object at 0x10dce7910>: Failed to establish a new connection: [Errno 61] Connection refused)
WARNING:elastic_transport.node_pool:Node <Urllib3HttpNode(http://localhost:9200)> has failed for 12 times in a row, putting on 30 second timeout```
Contributor
Author
There was a problem hiding this comment.
so this was 1) start up docker ES, 2) start up backend, 3) kill docker ES?
otherwise, we could come up with a contrived test case like "if es.ping() =.TRUE, pretend it failed" just to check the actual logging of errors even if the error is nonsense.
longshuicy
reviewed
Oct 26, 2022
backend/app/search/connect.py
Outdated
| else: | ||
| raise ServiceUnreachable("Elasticsearch") | ||
| except ServiceUnreachable as e: | ||
| log_error(e) |
Member
There was a problem hiding this comment.
i think you are missing an await here. log_error function is async. I set an break point here and it didn't actually enter this function :-(
Contributor
Author
There was a problem hiding this comment.
thank you, i added the await. we can talk today about how to test this
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.
Adds a function to catch exceptions in the DB with some extra info.