Skip to content
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

[BUG] Heathcheck timeout parameter not parsable #387

Closed
grechaw opened this issue May 12, 2023 · 2 comments · Fixed by #394
Closed

[BUG] Heathcheck timeout parameter not parsable #387

grechaw opened this issue May 12, 2023 · 2 comments · Fixed by #394
Labels
bug Something isn't working

Comments

@grechaw
Copy link
Contributor

grechaw commented May 12, 2023

What is the bug?

the OpenSearch client cannot change the default timeout for health checks.

The cluster endpoint for healthcheck requires a string representing time units:

http "os-morel-1:9200/_cluster/health?timeout=4s"

It fails if you supply an integer or float.

(rayish) √ deus_lex % http "os-morel-1:9200/_cluster/health?timeout=4.2"
HTTP/1.1 400 Bad Request
content-encoding: gzip
content-length: 172
content-type: application/json; charset=UTF-8

{
    "error": {
        "reason": "failed to parse setting [timeout] with value [4.2] as a time value: unit is missing or unrecognized",
        "root_cause": [
            {
                "reason": "failed to parse setting [timeout] with value [4.2] as a time value: unit is missing or unrecognized",
                "type": "illegal_argument_exception"
            }
        ],
        "type": "illegal_argument_exception"
    },
    "status": 400
}

However, the client call requires a numeric value and fails with a time unit string

x = OpenSearch(["http://os-morel-1:9200"])

x.cluster.health(timeout="4s")

ConnectionError: ConnectionError(Timeout value connect was b'4s', but it must be an int, float or None.) caused by: ValueError(Timeout value connect was b'4s', but it must be an int, float or None.)

x.cluster.health(timeout=4)

ConnectionError: ConnectionError(Timeout value connect was 4, but it must be an int, float or None.) caused by: ValueError(Timeout value connect was 4, but it must be an int, float or None.)

How can one reproduce the bug?

Steps to reproduce the behavior.

What is the expected behavior?

As with previous versions of elasticsearch client, I expect "timeout="4s" to work properly

What is your host/environment?

Operating system, version.

Do you have any screenshots?

If applicable, add screenshots to help explain your problem.

Do you have any additional context?

Add any other context about the problem.

@grechaw grechaw added bug Something isn't working untriaged Need triage labels May 12, 2023
@grechaw
Copy link
Contributor Author

grechaw commented May 12, 2023

I can see that the 'request_timeout' parameter can be used to get around this issue. I'm not sure if that's recommended practice or if this bug needs fixing.

@grechaw
Copy link
Contributor Author

grechaw commented May 16, 2023

This problem emerges from where the client encodes URL parameters as strings, which causes a validation error for the timeout parameter further down the stack.

I will open a PR that resolves the problem. I'll write a test too. I hope you will consider merging it.

grechaw added a commit to grechaw/opensearch-py that referenced this issue May 16, 2023
When the "timeout" parameter is escaped and turned into
a string, it does not pass validation checks.
This PR ensures that timeout is passed through as-is,
just like "request_timeout".
There's no explicit test for this parameter, but I included
it on some existing tests just to ensure that the code
is exercised.  Without the fix in place, one of the tests as modified will not pass.

Signed-off-by: Charles Greer <cgreer@lexmachina.com>
@wbeckler wbeckler removed the untriaged Need triage label May 22, 2023
dblock pushed a commit that referenced this issue May 23, 2023
* [#387] Do not escape the "timeout" parameter.

When the "timeout" parameter is escaped and turned into
a string, it does not pass validation checks.
This PR ensures that timeout is passed through as-is,
just like "request_timeout".
There's no explicit test for this parameter, but I included
it on some existing tests just to ensure that the code
is exercised.  Without the fix in place, one of the tests as modified will not pass.

Signed-off-by: Charles Greer <cgreer@lexmachina.com>

* Unit test for timeout parameter

Revert original test modifications.

Signed-off-by: Charles Greer <cgreer@lexmachina.com>

* Extend test and linter

Signed-off-by: Charles Greer <cgreer@lexmachina.com>

---------

Signed-off-by: Charles Greer <cgreer@lexmachina.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants