Fixes for the Avatica JDBC driver#12709
Conversation
|
Build and test on ARM64 CPU architecture (1) failed with: This failure is unrelated to this change. Let's ignore it. Build takes 10 hours. Restarting to fix an IT that expects too much detail in an error response. |
| * items can be filled in later as needed (except for the SQL | ||
| * and auth result, which are required.) | ||
| */ | ||
| public class SqlRequest |
There was a problem hiding this comment.
unsure on naming, seems a bit strange to me for something with request in the name to have things called result inside of it. I think since SqlQuery is the HTTP request object and there is a method to create one of these from one of those it adds to the perceived strangeness of the naming.
In some ways this seems like the JDBC analog of SqlQuery, though also with the auth information decorated on it, should we swap the names of SqlQuery and SqlRequest? or call this DruidQueryStatement .. something? idk naming is hard
Correctly implement regular and prepared statements Correctly implement result sets Fix race condition with contexts Clarify when parameters are used Prepare for single-pass through the planner
|
Renamed |
|
Ready for re-review. The build failed due to a flaky IT recorded in this issue. Otherwise, the code should be ready for final review. |
clintropolis
left a comment
clintropolis
left a comment
There was a problem hiding this comment.
👍
The only thing i'm unsure about is if the executor change for running stuff changes the characteristics of anything, but not sure how to find out without merging it to see
| throw DruidMeta.logFailure(new ISE("Too many open statements, limit is [%,d]", maxStatements)); | ||
| } | ||
|
|
||
| @SuppressWarnings("GuardedBy") |
There was a problem hiding this comment.
does this still need suppressed?
|
Release note item: In previous releases, Druid would automatically close the JDBC In this release, Druid's JDBC driver follows the JDBC standards more closely:
If you have code that depended on the old behavior, you may have to change your code to add the required close statement. |
3 participants
PR #12636 amends Druid to make a single pass through the Calcite planner. This was originally done in support of the catalog project to avoid resolving the same catalog object multiple times, which could encounter race conditions if the resolutions occurred on either side of a change to the catalog.
While discussing that change, @clintropolis called our attention to the "query optimized" way that Druid handles parameters, which then raised questions about how we ensure the proposed changes do not break our JDBC support. @gianm suggested we focus testing on JDBC.
That testing revealed that Druid's implementation of the JDBC protocol is a bit off, which may have led to some of the confusion around when to prepare and when to expect parameters. Issue #12682 and issue #12684 spell out the gaps.
While JDBC has two kinds of statements (
StatementandPreparedStatement), Druid has just one that attempts to do what the two JDBC statements do. That is, it tries to be both prepared and non-prepared.Also, while JDBC separates the idea of statement and
ResultSet, Druid combines the two: closing the result set closes the statement as well. While this might be handy, for aStatement, it does work at cross-purposes to thePreparedStatementwhich is designed to be prepared once and executed multiple times. Closing the statement after the first execution forcesPreparedStatementto be identical toStatement(except that the former takes parameters.)Corrects the Druid JDBC Driver Behavior
We could argue about the merits of the Druid design. However, JDBC is a standard and the correct behavior is spelled out in the JDBC 4.1 Spec. That behavior is:
Statementis a reusable object that can execute multiple SQL statements, using aResultSet. Closing theResultSetdoes not close the statement. Druid can, however, close theResultSeton EOF.PreparedStatementis a reusable object created with a (typically parameterized) SQL statement. Each execution is given a set of parameter values, and produces aResultSet. Again, closing theResultSetdoes not close the statement, which allows the statement to be executed with a different set of parameters.To correct the Druid behavior, the Avatica JDBC implementation is refactored to clearly model the JDBC concepts:
DruidJdbcStatementmodels a (non-prepared) Statement.DruidJdbcPreparedStatementmodels a PreparedStatement.AbstractDruidJdbcStatementis the common base class for the above.DruidJdbcResultSetrepresents a result set (a running query).While PR #12636 went on to modify the planner and the
SqlLifecylelayers, this PR leaves those layers unchanged, and implements the desired behavior with the existing code. The hope is that the result is a bit easier on reviewers. This code will be modified again later to support the single-pass planner.The Avatica driver tests were modified to be consistent with the JDBC standard. The original code didn't pass, but the revised code does.
The result is that we now have a clean SQL stack for JDBC, HTTP and internal use that make a single planner pass when possible, and use two passes for the JDBC prepare/execute model.
Flaky Test:
DruidAvaticaHandlerTest.testConcurrentQueries()Also fixes a flaky test:
DruidAvaticaHandlerTest.testConcurrentQueries(). This test has occasionally failed in builds. The problem was first mentioned in 2017 in Issue #4408 , which was claimed to be fixed. When run in a tight loop, however, the test eventually fails with an NPE. The issue is traced to the context on theDruidConnection: it is passed to each statement, where it is shared and concurrently modified. The fix is to make a copy for each new statement which avoids queries trying to share the same map.Verified the problem by running the test 100 times in a tight loop. Verified the fix the same way.
Minor Revisions
Careful inspection of the code identified several other improvements:
SqlRequestto hold the quad of (SQL, context, parameters, auth result), which makes function signatures simpler.DruidMetatoDruidConnection.DruidMetaresolves connections, then connections work with statements.This PR has: