-
Notifications
You must be signed in to change notification settings - Fork 27
Timeout setting for Opensearch and Elasticsearch #408
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
Conversation
@@ -56,6 +56,10 @@ def _es_config() -> Dict[str, Any]: | |||
if (u := os.getenv("ES_USER")) and (p := os.getenv("ES_PASS")): | |||
config["http_auth"] = (u, p) | |||
|
|||
# Include timeout setting if set | |||
if timeout := os.getenv("ES_TIMEOUT"): | |||
config["timeout"] = timeout |
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.
Hi. Nice addition. Should this be request_timeout
? https://www.elastic.co/guide/en/elasticsearch/client/python-api/8.18/config.html#timeouts
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.
Good catch, didn't notice the parameter was deprecated in Elasticsearch client, changed it
@@ -53,6 +53,10 @@ def _es_config() -> Dict[str, Any]: | |||
|
|||
config["headers"] = headers | |||
|
|||
# Include timeout setting if set | |||
if timeout := os.getenv("ES_TIMEOUT"): | |||
config["timeout"] = timeout |
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.
Is this right for Opensearch? I am not sure.
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.
It should be, from OpenSearch Client parameters:
"kwargs (Any) – any additional arguments will be passed on to the Transport class and, subsequently, to the Connection instances."
and then Connection has the timeout parameter:
"timeout (int) – default timeout in seconds (float, default: 10)"
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.
Thanks for looking into this, I was going through the opensearch-py code a little and this makes sense for sure.
@z-mrozu Can you add an entry here for the Readme? https://github.com/stac-utils/stac-fastapi-elasticsearch-opensearch?tab=readme-ov-file#configuration-reference |
@jonhealy1 just to double check - for opensearch if the timeout parameter is not set it defaults to 10s because of the value set in Connection class (both for the sync client and async client), but for Elasticsearch if the user doesn't set request_timeout it defaults to either urlib3's default timeout for sync client (because of the elastic-transport urlib3 transport implementation) - which would be None, I think - or elastic-transport's aiohttp default timeout for async client - which would be 10s - so I'm a bit unsure what to put in the Default column in configuration reference. Something like 10s (OS/ES async) None (ES sync) maybe? |
@z-mrozu I think maybe we say something general, because we don't actually set a default. We could leave it blank or say something like - uses db client default? |
@jonhealy1 I added es_timeout to the config reference and went for "DB client default" in Default column. I also cleaned up the formatting of the table a bit, hope that's okay |
This is from ai so I know it's questionable information but it is something that I am wondering about - are
I can try to verify this info when I have time. Are you a Elasticsearch or Opensearch User? |
Mostly Opensearch user, I think Also just checked and you can't set |
In earlier versions of ES (7.x) it seems that the |
The logic here - I guess - is that the Requests library will accept a tuple and there is nothing in the opensearch code that will prevent this? https://requests.readthedocs.io/en/latest/user/advanced/#timeouts Not my words: The timeout=(connect_timeout, read_timeout) tuple works in opensearch-py because the RequestsHttpConnection class (in opensearchpy/connection/http_requests.py) passes the timeout parameter directly to the requests library’s Session.send method, which supports a tuple for setting separate connection and read timeouts. The requests library interprets the tuple as (connect_timeout, read_timeout), applying the first value to establish the connection and the second to wait for the server’s response, ensuring both timeouts are enforced for all API calls despite the misleading Optional[Union[int, float]] type hint. |
I tested it, and by default the sync client of Opensearch uses urlib3 too and the Also, I double checked another thing while doing that and it seems that the sync client of Opensearch uses |
Yes, let's make that change. Thanks for doing all this work. After the readme change this should be good to go. |
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.
Thanks @z-mrozu!
Description:
Added timeout setting in Opensearch & Elasticsearch config which should only be relevant if user sets "ES_TIMEOUT"
PR Checklist:
pre-commit run --all-files
)make test
)