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

SQL: JDBC timeout properties are parsed in ms instead of seconds #79480

Closed
Luegg opened this issue Oct 19, 2021 · 1 comment · Fixed by #79628
Closed

SQL: JDBC timeout properties are parsed in ms instead of seconds #79480

Luegg opened this issue Oct 19, 2021 · 1 comment · Fixed by #79628
Labels
:Analytics/SQL SQL querying >bug Team:QL (Deprecated) Meta label for query languages team

Comments

@Luegg
Copy link
Contributor

Luegg commented Oct 19, 2021

The documentation for the ES JDBC driver states that all timeout system properties (like query.timeout) are parsed as seconds. This is currently not the case.

E.g. the following program

public class Main {
    public static void main(String[] args) {
        Properties props = new Properties();
        props.put("query.timeout", "10");
        props.put("page.timeout", "20");
        props.put("binary.format", "false");

        try (Connection conn = DriverManager.getConnection("jdbc:es://localhost:9202/", props);
             Statement stmt = conn.createStatement();
             ResultSet rs = stmt.executeQuery("SELECT 1");) {
            rs.next();
        } catch (SQLException e) {
            e.printStackTrace();
        }
    }
}

triggers a request with the body

    {
        "binary_format": false,
        "columnar": false,
        "field_multi_value_leniency": true,
        "keep_alive": "5d",
        "mode": "jdbc",
        "page_timeout": "10ms",
        "query": "SELECT 1",
        "request_timeout": "20ms",
        "time_zone": "Europe/Zurich",
        "version": "7.14.0"
    }

The same issue probably applies to the other timeout properties.

Also, the properties are treated as ms when read from the connection URL as in jdbc:es://localhost:9202/?query.timeout=10.

If the queryTimeout is set with Statement.setQueryTimeout it is interpreted in seconds as expected by the JDBC specs.

@Luegg Luegg added >bug :Analytics/SQL SQL querying labels Oct 19, 2021
@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Oct 19, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-ql (Team:QL)

Luegg pushed a commit to Luegg/elasticsearch that referenced this issue Oct 21, 2021
Luegg pushed a commit to Luegg/elasticsearch that referenced this issue Oct 25, 2021
Luegg pushed a commit that referenced this issue Oct 26, 2021
Resolves #79480

My initial thought was to change the properties to be interpreted as seconds but this might not be worth it. All relevant places in the code seem to assume the timeouts to be in ms and there does not seem to be a consistent use of ms or s across JDBC drivers (Postgres uses seconds, MySQL uses ms, MS SQL mixes the two depending on the connection property).

Hence, just fixing the docs might be easier.
Luegg pushed a commit to Luegg/elasticsearch that referenced this issue Oct 26, 2021
Resolves elastic#79480

My initial thought was to change the properties to be interpreted as seconds but this might not be worth it. All relevant places in the code seem to assume the timeouts to be in ms and there does not seem to be a consistent use of ms or s across JDBC drivers (Postgres uses seconds, MySQL uses ms, MS SQL mixes the two depending on the connection property).

Hence, just fixing the docs might be easier.
Luegg pushed a commit to Luegg/elasticsearch that referenced this issue Oct 26, 2021
Resolves elastic#79480

My initial thought was to change the properties to be interpreted as seconds but this might not be worth it. All relevant places in the code seem to assume the timeouts to be in ms and there does not seem to be a consistent use of ms or s across JDBC drivers (Postgres uses seconds, MySQL uses ms, MS SQL mixes the two depending on the connection property).

Hence, just fixing the docs might be easier.
elasticsearchmachine pushed a commit that referenced this issue Oct 26, 2021
Resolves #79480

My initial thought was to change the properties to be interpreted as seconds but this might not be worth it. All relevant places in the code seem to assume the timeouts to be in ms and there does not seem to be a consistent use of ms or s across JDBC drivers (Postgres uses seconds, MySQL uses ms, MS SQL mixes the two depending on the connection property).

Hence, just fixing the docs might be easier.
elasticsearchmachine pushed a commit that referenced this issue Oct 26, 2021
Resolves #79480

My initial thought was to change the properties to be interpreted as seconds but this might not be worth it. All relevant places in the code seem to assume the timeouts to be in ms and there does not seem to be a consistent use of ms or s across JDBC drivers (Postgres uses seconds, MySQL uses ms, MS SQL mixes the two depending on the connection property).

Hence, just fixing the docs might be easier.
Luegg pushed a commit that referenced this issue Oct 27, 2021
…uests (#79491)

* SQL: swap JDBC page.timeout and query.timeout properties in query requests

resolves #79430

* rm reference to #79480 from spec
Luegg pushed a commit to Luegg/elasticsearch that referenced this issue Oct 27, 2021
…uests (elastic#79491)

* SQL: swap JDBC page.timeout and query.timeout properties in query requests

resolves elastic#79430

* rm reference to elastic#79480 from spec
lockewritesdocs pushed a commit to lockewritesdocs/elasticsearch that referenced this issue Oct 28, 2021
Resolves elastic#79480

My initial thought was to change the properties to be interpreted as seconds but this might not be worth it. All relevant places in the code seem to assume the timeouts to be in ms and there does not seem to be a consistent use of ms or s across JDBC drivers (Postgres uses seconds, MySQL uses ms, MS SQL mixes the two depending on the connection property).

Hence, just fixing the docs might be easier.
Luegg pushed a commit to Luegg/elasticsearch that referenced this issue Dec 17, 2021
…uests (elastic#79491)

* SQL: swap JDBC page.timeout and query.timeout properties in query requests

resolves elastic#79430

* rm reference to elastic#79480 from spec
# Conflicts:
#	x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/JdbcHttpClient.java
#	x-pack/plugin/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/JdbcConfigurationDataSourceTests.java
elasticsearchmachine pushed a commit that referenced this issue Dec 20, 2021
…ery requests (#79491) (#81878)

* SQL: swap JDBC page.timeout and query.timeout properties in query requests (#79491)

* SQL: swap JDBC page.timeout and query.timeout properties in query requests

resolves #79430

* rm reference to #79480 from spec
# Conflicts:
#	x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/JdbcHttpClient.java
#	x-pack/plugin/sql/jdbc/src/test/java/org/elasticsearch/xpack/sql/jdbc/JdbcConfigurationDataSourceTests.java

* fix backport

* fix formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/SQL SQL querying >bug Team:QL (Deprecated) Meta label for query languages team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants