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

Ensure all connections (esp. JDBC) are closed on failure #151

Closed
1 task done
ckunki opened this issue Aug 18, 2023 · 5 comments · Fixed by #152
Closed
1 task done

Ensure all connections (esp. JDBC) are closed on failure #151

ckunki opened this issue Aug 18, 2023 · 5 comments · Fixed by #152
Assignees
Labels
refactoring Code improvement without behavior change

Comments

@ckunki
Copy link
Collaborator

ckunki commented Aug 18, 2023

Ticket mentioned as "IntRef" reported that when user cancels an SQL query in Exasol database the resulting Hive query in VSHIVE does not get canceled.

The current ticket therefore requests to inspect and if possible to improve the connection handling in VSCJDBC.
See also ticket exasol/hive-virtual-schema#39 for improved connection handling in VSHIVE.

If possible then VSCJDBC should ensure that especially in the case of an exception VSCJDBC closes all connections.

Acceptance criteria

  1. Connection handling in VSCJDBC is analysed and result of analysis is attached or linked to the current ticket
  2. If possible connection handling is improved

See for example ImportIntoTemporaryTableQueryRewriter.java#L75

private String createColumnsDescriptionFromQuery(final String query) throws SQLException {
    final ColumnMetadataReader columnMetadataReader = this.remoteMetadataReader.getColumnMetadataReader();
    final ResultSetMetadataReader resultSetMetadataReader = new ResultSetMetadataReader(
            this.connectionFactory.getConnection(), columnMetadataReader);
    final String columnsDescription = resultSetMetadataReader.describeColumns(query);
    LOGGER.finer(() -> "Import columns: " + columnsDescription);
    return columnsDescription;
}

Tasks

Preview Give feedback
  1. dependencies
    Shmuma
@ckunki ckunki added bug Unwanted / harmful behavior refactoring Code improvement without behavior change and removed bug Unwanted / harmful behavior labels Aug 18, 2023
@ckunki
Copy link
Collaborator Author

ckunki commented Aug 18, 2023

In my impression the factory is also used for caching and reusing a connection with the intention to improve performance by not creating a new connection every time.
From that closing the connection in case of an exception should rather be ensured on a higher level, e.g. in JDBCAdapter.pushdown().

@ckunki
Copy link
Collaborator Author

ckunki commented Aug 18, 2023

Alignment with @kaklakariada:

  • class JDBCAdapter in VSJDBC implements interface VirtualSchemaAdapter requiring 6 methods
  • each of these methods could happily create a new connection (or ConnectionFactory) and close it if an exception occurs as reusing a connection between multiple calls to one of these methods is not possible anyway.
  • we recommend to use an AutoCloseable to enable try-with-resources
  • for that probably RemoteConnectionFactory and interface ConnectionFactory need to be enhanced.

@Shmuma Shmuma self-assigned this Aug 21, 2023
@Shmuma
Copy link
Contributor

Shmuma commented Sep 11, 2023

Couple of issues in connection management have found:

  1. JDBC connections are AutoClosable interface, so they will be closed on try-with-resource block. But we're not using this pattern anywhere, so connections will be closed only on garbage collection (because we're not closing them explicitly as well)
  2. RemoteConnectionFactory was created inside the function: https://github.com/exasol/virtual-schema-common-jdbc/blob/main/src/main/java/com/exasol/adapter/jdbc/JDBCAdapter.java#L161, which is not how factories should be created. Because of that, connections are automatically closed after every operation (query or schema refresh) and being reestablished again. It also might lead to connection leak in some cases (if reference is kept after exception, for example)
  3. on exception we're not closing connection explicitly

@Shmuma
Copy link
Contributor

Shmuma commented Sep 11, 2023

But before fixing all this, hive-virtual-schema has to be updated to the latest libraries version

@Shmuma
Copy link
Contributor

Shmuma commented Sep 12, 2023

Optimizing connection establishment doesn't have much sense, as we're restarting whole java process on every request (at least jar file is being reloaded from bucketfs)

@kaklakariada kaklakariada self-assigned this Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code improvement without behavior change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants