Copilot commented on code in PR #7148:
URL: https://github.com/apache/ignite-3/pull/7148#discussion_r2588981367
##########
modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcConnectionSelfTest.java:
##########
@@ -1049,6 +1050,117 @@ public void testChangeBackgroundReconnectInterval()
throws SQLException {
format("Failed to parse int property [name={},
value=9223372036854775808]", propertyName));
}
+ @Test
+ public void testQueryTimeoutProperty() throws SQLException {
+ String propertyName = "queryTimeoutSeconds";
+ String urlPrefix = URL + "?" + propertyName;
+
+ SqlThrowingFunction<String, Number> valueGetter = url -> {
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ try (Statement stmt = conn.createStatement();
+ PreparedStatement pstmt =
conn.prepareStatement("SELECT 1")) {
+
+ assertThat(stmt.getQueryTimeout(),
is(pstmt.getQueryTimeout()));
+
+ return stmt.getQueryTimeout();
+ }
+ }
+ };
+
+ assertThat(valueGetter.apply(URL), is(0));
+ assertThat(valueGetter.apply(urlPrefix + "=2147483647"),
is(Integer.MAX_VALUE));
+ assertThat(valueGetter.apply(urlPrefix + "=0"), is(0));
+
+ assertInvalid(urlPrefix + "=A",
+ format("Failed to parse int property [name={}, value=A]",
propertyName));
+
+ assertInvalid(urlPrefix + "=-1",
+ format("Property cannot be lower than 0 [name={}, value=-1]",
propertyName));
+
+ assertInvalid(urlPrefix + "=2147483648",
+ format("Failed to parse int property [name={},
value=2147483648]", propertyName));
+
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(urlPrefix + "=1")) {
+ try (Statement stmt0 = conn.createStatement()) {
+ // Catch both execution and planning timeouts.
+ assertThrowsSqlException(SQLException.class,
+ "timeout", () -> {
+ try (ResultSet rs = stmt0.executeQuery("SELECT *
FROM TABLE(SYSTEM_RANGE(1, 100000000))")) {
+ while (rs.next()) {
+ rs.getLong(1);
+ }
+ }
+ });
+ }
+ }
+
+ // Check legacy name "queryTimeout".
+ {
+ {
+ String url = URL + "?queryTimeout=100";
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ assertThat(conn.properties().getQueryTimeout(), is(100));
+ }
+ }
+
+ { // If both are specified, the deprecated property should be
ignored.
+ String url = URL + "?queryTimeout=100&queryTimeoutSeconds=50";
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ assertThat(conn.properties().getQueryTimeout(), is(50));
+ }
+ }
Review Comment:
[nitpick] Unnecessary nested scope blocks (lines 1099-1104 and 1106-1111).
The inner braces don't provide any benefit here and add visual clutter.
Consider removing the extra nesting level:
```java
// Check legacy name "queryTimeout".
{
String url = URL + "?queryTimeout=100";
try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
assertThat(conn.properties().getQueryTimeout(), is(100));
}
// If both are specified, the deprecated property should be ignored.
String url2 = URL + "?queryTimeout=100&queryTimeoutSeconds=50";
try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url2)) {
assertThat(conn.properties().getQueryTimeout(), is(50));
}
}
```
```suggestion
String url = URL + "?queryTimeout=100";
try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
assertThat(conn.properties().getQueryTimeout(), is(100));
}
// If both are specified, the deprecated property should be ignored.
String url2 = URL + "?queryTimeout=100&queryTimeoutSeconds=50";
try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url2)) {
assertThat(conn.properties().getQueryTimeout(), is(50));
```
##########
modules/jdbc/src/main/java/org/apache/ignite/internal/jdbc/ConnectionPropertiesImpl.java:
##########
@@ -58,18 +58,39 @@ public class ConnectionPropertiesImpl implements
ConnectionProperties, Serializa
private final StringProperty schema = new StringProperty(PROP_SCHEMA,
"Schema name of the connection", "PUBLIC", null, false, null);
- /** Query timeout. */
+ /**
+ * Query timeout.
+ *
+ * @deprecated Use {@link #qryTimeoutSeconds} instead.
+ */
Review Comment:
The deprecated `qryTimeout` property (line 66) is missing the `@Deprecated`
annotation, while the similarly deprecated `connTimeout` property has it on
line 82. For consistency, add the `@Deprecated` annotation to `qryTimeout` as
well.
```suggestion
*/
@Deprecated
```
##########
modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcConnectionSelfTest.java:
##########
@@ -1049,6 +1050,117 @@ public void testChangeBackgroundReconnectInterval()
throws SQLException {
format("Failed to parse int property [name={},
value=9223372036854775808]", propertyName));
}
+ @Test
+ public void testQueryTimeoutProperty() throws SQLException {
+ String propertyName = "queryTimeoutSeconds";
+ String urlPrefix = URL + "?" + propertyName;
+
+ SqlThrowingFunction<String, Number> valueGetter = url -> {
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ try (Statement stmt = conn.createStatement();
+ PreparedStatement pstmt =
conn.prepareStatement("SELECT 1")) {
+
+ assertThat(stmt.getQueryTimeout(),
is(pstmt.getQueryTimeout()));
+
+ return stmt.getQueryTimeout();
+ }
+ }
+ };
+
+ assertThat(valueGetter.apply(URL), is(0));
+ assertThat(valueGetter.apply(urlPrefix + "=2147483647"),
is(Integer.MAX_VALUE));
+ assertThat(valueGetter.apply(urlPrefix + "=0"), is(0));
+
+ assertInvalid(urlPrefix + "=A",
+ format("Failed to parse int property [name={}, value=A]",
propertyName));
+
+ assertInvalid(urlPrefix + "=-1",
+ format("Property cannot be lower than 0 [name={}, value=-1]",
propertyName));
+
+ assertInvalid(urlPrefix + "=2147483648",
+ format("Failed to parse int property [name={},
value=2147483648]", propertyName));
+
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(urlPrefix + "=1")) {
+ try (Statement stmt0 = conn.createStatement()) {
+ // Catch both execution and planning timeouts.
+ assertThrowsSqlException(SQLException.class,
+ "timeout", () -> {
+ try (ResultSet rs = stmt0.executeQuery("SELECT *
FROM TABLE(SYSTEM_RANGE(1, 100000000))")) {
+ while (rs.next()) {
+ rs.getLong(1);
+ }
+ }
+ });
+ }
+ }
+
+ // Check legacy name "queryTimeout".
+ {
+ {
+ String url = URL + "?queryTimeout=100";
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ assertThat(conn.properties().getQueryTimeout(), is(100));
+ }
+ }
+
+ { // If both are specified, the deprecated property should be
ignored.
+ String url = URL + "?queryTimeout=100&queryTimeoutSeconds=50";
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ assertThat(conn.properties().getQueryTimeout(), is(50));
+ }
+ }
+ }
+ }
+
+ @Test
+ public void testConnectionTimeoutProperty() throws SQLException {
+ String propertyName = "connectionTimeoutMillis";
+ String urlPrefix = URL + "?" + propertyName;
+
+ SqlThrowingFunction<String, Number> valueGetter = url -> {
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ return conn.properties().getConnectionTimeout();
+ }
+ };
+
+ assertThat(valueGetter.apply(URL), is(0));
+ assertThat(valueGetter.apply(urlPrefix + "=2147483647"),
is(Integer.MAX_VALUE));
+ assertThat(valueGetter.apply(urlPrefix + "=0"), is(0));
+
+ assertInvalid(urlPrefix + "=A",
+ format("Failed to parse int property [name={}, value=A]",
propertyName));
+
+ assertInvalid(urlPrefix + "=-1",
+ format("Property cannot be lower than 0 [name={}, value=-1]",
propertyName));
+
+ assertInvalid(urlPrefix + "=2147483648",
+ format("Failed to parse int property [name={},
value=2147483648]", propertyName));
+
+ // Check legacy name "connectionTimeout".
+ {
+ {
+ String url = URL + "?connectionTimeout=100";
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ assertThat(conn.properties().getConnectionTimeout(),
is(100));
+ }
+ }
+
+ { // If both are specified, the deprecated property should be
ignored.
+ String url = URL +
"?connectionTimeout=100&connectionTimeoutMillis=50";
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ assertThat(conn.properties().getConnectionTimeout(),
is(50));
+ }
+ }
+
+ { // If both are specified, the deprecated property should be
ignored.
Review Comment:
[nitpick] The comment "If both are specified, the deprecated property should
be ignored." is duplicated on lines 1148 and 1155. Both test blocks are testing
the same scenario (that the new property takes precedence), just with
parameters in different orders. Consider differentiating the comments to
clarify what each test case specifically validates, for example:
- Line 1148: "If both are specified, the new property takes precedence
(deprecated first)"
- Line 1155: "If both are specified, the new property takes precedence (new
first)"
```suggestion
{ // If both are specified, the new property takes precedence
(deprecated first).
String url = URL +
"?connectionTimeout=100&connectionTimeoutMillis=50";
try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
assertThat(conn.properties().getConnectionTimeout(),
is(50));
}
}
{ // If both are specified, the new property takes precedence
(new first).
```
##########
modules/jdbc/src/integrationTest/java/org/apache/ignite/jdbc/ItJdbcConnectionSelfTest.java:
##########
@@ -1049,6 +1050,117 @@ public void testChangeBackgroundReconnectInterval()
throws SQLException {
format("Failed to parse int property [name={},
value=9223372036854775808]", propertyName));
}
+ @Test
+ public void testQueryTimeoutProperty() throws SQLException {
+ String propertyName = "queryTimeoutSeconds";
+ String urlPrefix = URL + "?" + propertyName;
+
+ SqlThrowingFunction<String, Number> valueGetter = url -> {
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ try (Statement stmt = conn.createStatement();
+ PreparedStatement pstmt =
conn.prepareStatement("SELECT 1")) {
+
+ assertThat(stmt.getQueryTimeout(),
is(pstmt.getQueryTimeout()));
+
+ return stmt.getQueryTimeout();
+ }
+ }
+ };
+
+ assertThat(valueGetter.apply(URL), is(0));
+ assertThat(valueGetter.apply(urlPrefix + "=2147483647"),
is(Integer.MAX_VALUE));
+ assertThat(valueGetter.apply(urlPrefix + "=0"), is(0));
+
+ assertInvalid(urlPrefix + "=A",
+ format("Failed to parse int property [name={}, value=A]",
propertyName));
+
+ assertInvalid(urlPrefix + "=-1",
+ format("Property cannot be lower than 0 [name={}, value=-1]",
propertyName));
+
+ assertInvalid(urlPrefix + "=2147483648",
+ format("Failed to parse int property [name={},
value=2147483648]", propertyName));
+
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(urlPrefix + "=1")) {
+ try (Statement stmt0 = conn.createStatement()) {
+ // Catch both execution and planning timeouts.
+ assertThrowsSqlException(SQLException.class,
+ "timeout", () -> {
+ try (ResultSet rs = stmt0.executeQuery("SELECT *
FROM TABLE(SYSTEM_RANGE(1, 100000000))")) {
+ while (rs.next()) {
+ rs.getLong(1);
+ }
+ }
+ });
+ }
+ }
+
+ // Check legacy name "queryTimeout".
+ {
+ {
+ String url = URL + "?queryTimeout=100";
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ assertThat(conn.properties().getQueryTimeout(), is(100));
+ }
+ }
+
+ { // If both are specified, the deprecated property should be
ignored.
+ String url = URL + "?queryTimeout=100&queryTimeoutSeconds=50";
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ assertThat(conn.properties().getQueryTimeout(), is(50));
+ }
+ }
+ }
+ }
+
+ @Test
+ public void testConnectionTimeoutProperty() throws SQLException {
+ String propertyName = "connectionTimeoutMillis";
+ String urlPrefix = URL + "?" + propertyName;
+
+ SqlThrowingFunction<String, Number> valueGetter = url -> {
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ return conn.properties().getConnectionTimeout();
+ }
+ };
+
+ assertThat(valueGetter.apply(URL), is(0));
+ assertThat(valueGetter.apply(urlPrefix + "=2147483647"),
is(Integer.MAX_VALUE));
+ assertThat(valueGetter.apply(urlPrefix + "=0"), is(0));
+
+ assertInvalid(urlPrefix + "=A",
+ format("Failed to parse int property [name={}, value=A]",
propertyName));
+
+ assertInvalid(urlPrefix + "=-1",
+ format("Property cannot be lower than 0 [name={}, value=-1]",
propertyName));
+
+ assertInvalid(urlPrefix + "=2147483648",
+ format("Failed to parse int property [name={},
value=2147483648]", propertyName));
+
+ // Check legacy name "connectionTimeout".
+ {
+ {
+ String url = URL + "?connectionTimeout=100";
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ assertThat(conn.properties().getConnectionTimeout(),
is(100));
+ }
+ }
+
+ { // If both are specified, the deprecated property should be
ignored.
+ String url = URL +
"?connectionTimeout=100&connectionTimeoutMillis=50";
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ assertThat(conn.properties().getConnectionTimeout(),
is(50));
+ }
+ }
+
+ { // If both are specified, the deprecated property should be
ignored.
+ String url = URL +
"?connectionTimeoutMillis=50&connectionTimeout=100";
+ try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url)) {
+ assertThat(conn.properties().getConnectionTimeout(),
is(50));
+ }
+ }
Review Comment:
[nitpick] Unnecessary nested scope blocks (lines 1141-1146, 1148-1153, and
1155-1160). The inner braces don't provide any benefit here and add visual
clutter. Consider removing the extra nesting level and combining similar test
cases more clearly.
```suggestion
// Check legacy name "connectionTimeout".
String url1 = URL + "?connectionTimeout=100";
try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url1)) {
assertThat(conn.properties().getConnectionTimeout(), is(100));
}
// If both are specified, the deprecated property should be ignored.
String url2 = URL +
"?connectionTimeout=100&connectionTimeoutMillis=50";
try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url2)) {
assertThat(conn.properties().getConnectionTimeout(), is(50));
}
// If both are specified, the deprecated property should be ignored.
String url3 = URL +
"?connectionTimeoutMillis=50&connectionTimeout=100";
try (JdbcConnection conn = (JdbcConnection)
DriverManager.getConnection(url3)) {
assertThat(conn.properties().getConnectionTimeout(), is(50));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
