Adding request-timeout for recall metrics for so_vector #800
Conversation
| @@ -200,6 +204,7 @@ async def __call__(self, es, params): | |||
| k = params["size"] | |||
| num_candidates = params["num_candidates"] | |||
| index = params["index"] | |||
| request_timeout = params.get("request-timeout", -1) | |||
There was a problem hiding this comment.
I don't know the appropriate default value to indicate "no timeout". I think for these recall metrics only, that is what we care about as these types of queries (script & knn) are performance tested elsewhere. So if there is a significant regression, we will catch it in those other operations.
There was a problem hiding this comment.
Ah, was looking at exactly the same line :)
| @@ -200,6 +204,7 @@ async def __call__(self, es, params): | |||
| k = params["size"] | |||
| num_candidates = params["num_candidates"] | |||
| index = params["index"] | |||
| request_timeout = params.get("request-timeout", -1) | |||
There was a problem hiding this comment.
Looking at https://www.elastic.co/docs/reference/elasticsearch/clients/python/configuration#timeouts it's not clear to me what's the behavior with a negative value. Did it work in your tests?
I think it would be safer to either use None (no timeout) or some positive value (some timeout). If you're OK with defaulting to no timeout, we could just use None here, instead of -1.
Alternatively, to allow setting either None or some positive value, or not setting anything (and thus defaulting to Rally-level timeout):
DEFAULT = 0
es_kwargs = {}
request_timeout = params.get("request-timeout", DEFAULT)
if request_timeout != DEFAULT:
es_kwargs["request_timeout"] = request_timeout
[..]
await es.<method>(..., **es_kwargs).
we need to adjust timeout for recall tasks for so_vector.