clickhouse-java icon indicating copy to clipboard operation
clickhouse-java copied to clipboard

ClickHouseConnectionImpl.isValid

Open bgranvea opened this issue 6 years ago • 1 comments

I don't understand the implementation of isValid in ClickHouseConnectionImpl:

    @Override
    public boolean isValid(int timeout) throws SQLException {
        if (isClosed()) {
            return false;
        }

        try {
            ClickHouseProperties properties = new ClickHouseProperties(this.properties);
            properties.setConnectionTimeout((int) TimeUnit.SECONDS.toMillis(timeout));
            CloseableHttpClient client = new ClickHouseHttpClientBuilder(properties).buildClient();
            Statement statement = createClickHouseStatement(client);
            statement.execute("SELECT 1");
            statement.close();
            return true;
        } catch (Exception e) {
            boolean isFailOnConnectionTimeout = e.getCause() instanceof ConnectTimeoutException;
            if (!isFailOnConnectionTimeout) {
                log.warn("Something had happened while validating a connection", e);
            }

            return false;
        }
    }

I don't understand why you need to create a new HTTP client to test a connection to the server. Why don't you simply use the existing CloseableHttpClient? Something like:

        Statement statement = null;
        try {
            statement = createStatement();
            statement.execute("SELECT 1");
            return true;
        } catch (Exception e) {
            (...)
            return false;
        } finally {
        	if (statement != null)
        		statement.close();
        }

If you keep the new CloseableHttpClient, shouldn't it be released at the end with client.close() ?

bgranvea avatar Jan 15 '19 09:01 bgranvea

@bgranvea This is a good question.

We create a new HTTP client because using an existed HTTP client isn't appropriate here. An existed client could have a timeout connection parameter. However, a validation request has been performed with a set timeout parameter. These parameters can be different. Apparently, we have to create a new HTTP client each time.

izebit avatar Jun 19 '19 15:06 izebit