review fixes

src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java

@@ -3252,7 +3252,7 @@ private void login(String primary, String primaryInstanceName, int primaryPortNu
         int fedauthRetryInterval = BACKOFF_INTERVAL; // milliseconds to sleep (back off) between attempts.
 
         long timeoutUnitInterval;
-        long timeForFirstTry = 0; // time it took to do 1st try
+        long timeForFirstTry = 0; // time it took to do 1st try in ms
 
         boolean useFailoverHost = false;
         FailoverInfo tempFailover = null;
@@ -3464,7 +3464,7 @@ private void login(String primary, String primaryInstanceName, int primaryPortNu
                         || (connectRetryCount == 0 && !isDBMirroring && !useTnir) // retries disabled
                         // retry at least once for TNIR and failover
                         || (connectRetryCount == 0 && (isDBMirroring || useTnir) && attemptNumber > 0)
-                        || (connectRetryCount != 0 && attemptNumber > connectRetryCount) // no retries left
+                        || (connectRetryCount != 0 && attemptNumber >= connectRetryCount) // no retries left
                 ) {
                     if (loggerResiliency.isLoggable(Level.FINER)) {
                         logConnectFailure(attemptNumber, e, sqlServerError);
@@ -3505,14 +3505,14 @@ private void login(String primary, String primaryInstanceName, int primaryPortNu
             // We only get here when we failed to connect, but are going to re-try
             // After trying to connect to both servers fails, sleep for a bit to prevent clogging
             // the network with requests, then update sleep interval for next iteration (max 1 second interval)
-            // We have to sleep for every attempt in case of non-dbMirroring scenarios (including multi subnetfailover),
+            // We have to sleep for every attempt in case of non-dbMirroring scenarios (including multisubnetfailover),
             // Whereas for dbMirroring, we sleep for every two attempts as each attempt is to a different server.
             // Make sure there's enough time to do another retry
-            if (!isDBMirroring && (0 == attemptNumber % 2) || (isDBMirroring && attemptNumber < 1)
-                    || (attemptNumber < connectRetryCount && connectRetryCount != 0) && timerRemaining(
-                            timerExpire) > (TimeUnit.SECONDS.toMillis(connectRetryInterval) + 2 * timeForFirstTry))
+            if (!isDBMirroring || (isDBMirroring && (0 == attemptNumber % 2))
+                    && (attemptNumber < connectRetryCount && connectRetryCount != 0) 
+                    && timerRemaining(
+                            timerExpire) > (TimeUnit.SECONDS.toMillis(connectRetryInterval) + 2 * timeForFirstTry)) {
 
-            {
                 // don't wait for TNIR
                 if (!(useTnir && attemptNumber == 0)) {
                     if (loggerResiliency.isLoggable(Level.FINER)) {

src/test/java/com/microsoft/sqlserver/jdbc/SQLServerConnectionTest.java

@@ -500,7 +500,7 @@ public void testConnectTnir() {
 
         // no retries but should connect to TNIR (this assumes host is defined in host file
         try (Connection con = PrepUtil
-                .getConnection(connectionString + ";transparentNetworkIPResolution=true;conneectRetry=0;serverName="
+                .getConnection(connectionString + ";transparentNetworkIPResolution=true;connectRetryCount=0;serverName="
                         + tnirHost);) {} catch (Exception e) {
             fail(e.getMessage());
         }
@@ -516,7 +516,7 @@ public void testConnectNoTnir() {
 
         // no retries no TNIR should fail even tho host is defined in host file
         try (Connection con = PrepUtil.getConnection(
-                connectionString + ";transparentNetworkIPResolution=false;conneectRetry=0;serverName=" + tnirHost);) {
+                connectionString + ";transparentNetworkIPResolution=false;connectRetryCount=0;serverName=" + tnirHost);) {
             assertTrue(con == null, TestResource.getResource("R_shouldNotConnect"));
         } catch (Exception e) {
             assertTrue(e.getMessage().matches(TestUtils.formatErrorMsg("R_tcpipConnectionFailed"))

src/test/java/com/microsoft/sqlserver/jdbc/connection/TimeoutTest.java

@@ -35,7 +35,6 @@
 
 
 @RunWith(JUnitPlatform.class)
-@Tag("slow")
 public class TimeoutTest extends AbstractTest {
     static String randomServer = RandomUtil.getIdentifier("Server");
     static String waitForDelaySPName = RandomUtil.getIdentifier("waitForDelaySP");
@@ -47,6 +46,13 @@ public static void setupTests() throws Exception {
         setConnection();
     }
 
+    /*
+     * TODO:
+     * The tests below uses a simple interval counting logic to determine whether there was at least 1 retry.
+     * Given the interval is long enough, then 1 retry should take at least 1 interval long, so if it took < 1 interval, then it assumes there were no retry. However, this only works if TNIR or failover is not enabled since those cases should retry but no wait interval in between. So this interval counting can not detect these cases.
+     * Note a better and more reliable way would be to check attemptNumber using reflection to determine the number of retries.
+     */
+
     // test default loginTimeout used if not specified in connection string
     @Test
     public void testDefaultLoginTimeout() {
@@ -337,36 +343,6 @@ public void testAzureEndpointRetry() {
         }
     }
 
-    // test failover retries once even if connectRetry is 0
-    @Test
-    public void testFailoverInstanceResolution() throws SQLException {
-        long totalTime = 0;
-        long timerStart = System.currentTimeMillis();
-        int interval = defaultTimeout / 2; // interval long enough to tell if there was a retry
-        long loginTimeout = defaultTimeout; // long enough loginTimeout to accommodate the interval
-
-        // non-existent server, retry 0 but failover enabled so there should be 1 retry
-        try (Connection con = PrepUtil
-                .getConnection("jdbc:sqlserver://" + randomServer + ";databaseName=FailoverDB_abc;failoverPartner="
-                        + randomServer + ";user=sa;password=" + RandomUtil.getIdentifier("password") + ";loginTimeout="
-                        + loginTimeout + ";connectRetryCount=0;connectRetryInterval=" + interval)) {
-            fail(TestResource.getResource("R_shouldNotConnect"));
-        } catch (Exception e) {
-            totalTime = System.currentTimeMillis() - timerStart;
-
-            assertTrue(
-                    (e.getMessage().toLowerCase()
-                            .contains(TestResource.getResource("R_tcpipConnectionToHost").toLowerCase()))
-                            || ((isSqlAzure() || isSqlAzureDW()) ? e.getMessage().toLowerCase()
-                                    .contains(TestResource.getResource("R_connectTimedOut").toLowerCase()) : false),
-                    e.getMessage());
-        }
-
-        // if there was a retry then it would be at least 1 interval long
-        assertTrue(TimeUnit.SECONDS.toMillis(interval) < totalTime,
-                "interval: " + TimeUnit.SECONDS.toMillis(interval) + " total time: " + totalTime);
-    }
-
     /**
      * When query timeout occurs, the connection is still usable.
      *