fix: query timeout kill query directed to incorrect instance #474
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
crystall-bitquill
force-pushed
the
461
branch
from
bc6f795 to
3ab271b
Compare
sergiyvamz
reviewed
Oct 16, 2023
| @@ -607,4 +618,16 @@ public void test_PreparedStatementHashCodes() throws SQLException, IOException { | |||
| conn.close(); | |||
| } | |||
|
|
|||
| @RepeatedTest(100) | |||
There was a problem hiding this comment.
Why do we need to run it so many times?
There was a problem hiding this comment.
When the new connection for the query timeout is made, the reader cluster endpoint doesn't always choose a reader different from the original connection so it's hard to reproduce this issue consistently.
sergiyvamz
reviewed
Oct 16, 2023
Comment on lines
113
to
126
| final RdsUtils rdsUtils = new RdsUtils(); | ||
| if (rdsUtils.isReaderClusterDns(mainHostInfo.getHost())) { | ||
| final String connectedHostName = getCurrentlyConnectedInstance(connection); | ||
| String instanceEndpoint = | ||
| rdsUtils.getRdsInstanceHostPattern(mainHostInfo.getHost()).replace("?", connectedHostName); | ||
| if (!StringUtils.isNullOrEmpty(mainHostInfo.getHostProperties().get(PropertyKey.clusterInstanceHostPattern.getKeyName()))) { | ||
| instanceEndpoint = mainHostInfo.getHostProperties().get(PropertyKey.clusterInstanceHostPattern.getKeyName()) | ||
| .replace("?", connectedHostName); | ||
| } | ||
| final HostInfo instanceHostInfo = ConnectionUtils.createInstanceHostWithProperties(instanceEndpoint, mainHostInfo); | ||
| final JdbcConnection hostConnection = this.connectionProvider.connect(instanceHostInfo); | ||
| connection.close(); | ||
| connection = hostConnection; | ||
| } |
There was a problem hiding this comment.
I'd suggest to relocate this logic into a new plugin. In this way, it'd be easier for users to use it when it's needed, or skip it. That gives more flexibility for users who don't experience any issue with query timeout.
sergiyvamz
reviewed
Oct 16, 2023
| * http://www.gnu.org/licenses/gpl-2.0.html. | ||
| */ | ||
|
|
||
| package com.mysql.cj.jdbc.ha; |
There was a problem hiding this comment.
com.mysql.cj.jdbc.ha.util ?
There was a problem hiding this comment.
And similar suggestion's for the ConnectionUtils class above.
crystall-bitquill
force-pushed
the
461
branch
3 times, most recently
from
09c684b to
b846c4b
Compare
sergiyvamz
approved these changes
Oct 30, 2023
crystall-bitquill
force-pushed
the
461
branch
from
b846c4b to
07aced7
Compare
karenc-bq
reviewed
Oct 31, 2023
| IConnectionPlugin nextPlugin, | ||
| Log logger, | ||
| IConnectionProvider connectionProvider) { | ||
| if (logger == null) { |
There was a problem hiding this comment.
I don't think logger is used in this file. Do we need to store it?
karenc-bq
reviewed
Oct 31, 2023
Comment on lines
112
to
120
| String instanceEndpoint = | ||
| rdsUtils.getRdsInstanceHostPattern(mainHostInfo.getHost()).replace("?", connectedHostName); | ||
| if (!StringUtils.isNullOrEmpty( | ||
| mainHostInfo.getHostProperties().get(PropertyKey.clusterInstanceHostPattern.getKeyName()))) { | ||
| instanceEndpoint = mainHostInfo.getHostProperties().get(PropertyKey.clusterInstanceHostPattern.getKeyName()) | ||
| .replace("?", connectedHostName); | ||
| } | ||
| final HostInfo instanceHostInfo = | ||
| ConnectionUtils.createInstanceHostWithProperties(instanceEndpoint, mainHostInfo); |
There was a problem hiding this comment.
Suggested change
| String instanceEndpoint = | |
| rdsUtils.getRdsInstanceHostPattern(mainHostInfo.getHost()).replace("?", connectedHostName); | |
| if (!StringUtils.isNullOrEmpty( | |
| mainHostInfo.getHostProperties().get(PropertyKey.clusterInstanceHostPattern.getKeyName()))) { | |
| instanceEndpoint = mainHostInfo.getHostProperties().get(PropertyKey.clusterInstanceHostPattern.getKeyName()) | |
| .replace("?", connectedHostName); | |
| } | |
| final HostInfo instanceHostInfo = | |
| ConnectionUtils.createInstanceHostWithProperties(instanceEndpoint, mainHostInfo); | |
| final String pattern = mainHostInfo.getHostProperties().get(PropertyKey.clusterInstanceHostPattern.getKeyName()); | |
| final String instanceEndpoint = !StringUtils.isNullOrEmpty(pattern) ? pattern : rdsUtils.getRdsInstanceHostPattern(mainHostInfo.getHost()); | |
| final HostInfo instanceHostInfo = | |
| ConnectionUtils.createInstanceHostWithProperties( | |
| instanceEndpoint.replace("?", connectedHostName), | |
| mainHostInfo); |
karenc-bq
reviewed
Oct 31, 2023
Comment on lines
145
to
146
| rs.next(); | ||
| return rs.getString(1); |
There was a problem hiding this comment.
Suggested change
| rs.next(); | |
| return rs.getString(1); | |
| if (rs.next()) { | |
| return rs.getString(1); | |
| } | |
| return null; | |
| } |
karenc-bq
approved these changes
Oct 31, 2023
crystall-bitquill
force-pushed
the
461
branch
from
07aced7 to
15403f8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
fix: query timeout kill query directed to incorrect instance
Description
When a query timeout is set and the timeout value has passed, the driver sends a kill query to stop the process through a new connection that was created using the same host as the original connection. When using the reader cluster url, connections are randomly sent to one of the available reader instances. This will occasionally cause errors when the kill query is sent to a different instance where the statement was not executed, resulting in an "Unknown process id" message.
This PR adds a check in the default connection plugin for connections using the reader cluster url. Once the connection is made using the reader cluster url, a new connection is made to a reader instance that is obtained by querying for the current instance name. The original connection using the reader cluster is replaced with the connection to the instance.
Addresses #461
Additional Reviewers