[GitHub] [ignite] korlov42 commented on a change in pull request #8107: IGNITE-13183 Query timeout redesign

[GitBox <gi...@apache.org>]

korlov42 commented on a change in pull request #8107:
URL: https://github.com/apache/ignite/pull/8107#discussion_r472089059



##########
File path: modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinConnection.java
##########
@@ -1046,6 +1048,29 @@ else if (res.status() != ClientListenerResponse.STATUS_SUCCESS)
         }
     }
 
+    /**
+     * Get timeout and node information

Review comment:
       missing dots

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinStatement.java
##########
@@ -498,9 +501,7 @@ void closeOnDisconnect() {
         if (timeout < 0)
             throw new SQLException("Invalid timeout value.");
 
-        this.timeout = timeout * 1000;
-
-        reqTimeout = this.timeout;
+        timeout(timeout * 1000 > timeout ? timeout * 1000 : Integer.MAX_VALUE);

Review comment:
       `timeout * 1000 > timeout`... It means timeout is positive?

##########
File path: modules/clients/src/test/java/org/apache/ignite/jdbc/thin/JdbcThinDefaultTimeoutTest.java
##########
@@ -91,20 +100,27 @@ public void testDefaultTimeoutIgnored() throws Exception {
     }
 
     /**
-     * Utility class with custom SQL functions.
+     * Check JDBC query timeout.
+     * Steps:
+     * - set default timeout to 100 ms;
+     * - execute query without explicit timeout;
+     * - check that query fails by timeout.

Review comment:
       Well, you don't set default timeout in the test actually, so
   ```suggestion
        * - execute query with implicit default timeout {@link DEFAULT_TIMEOUT} <-- please create a const for this;
        * - check that query fails by timeout.
   ```




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org