From fc20b85859f19b2f9b66d0a3c95925bd69f4c213 Mon Sep 17 00:00:00 2001 From: lilgreenbird Date: Wed, 28 Feb 2024 12:52:30 -0800 Subject: [PATCH] review fixes --- .../sqlserver/jdbc/SQLServerConnection.java | 14 +++---- .../jdbc/SQLServerConnectionTest.java | 4 +- .../jdbc/connection/TimeoutTest.java | 38 ++++--------------- 3 files changed, 16 insertions(+), 40 deletions(-) diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java b/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java index d5689ce816..982cf0e8ed 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java +++ b/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)) { diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/SQLServerConnectionTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/SQLServerConnectionTest.java index 2d16d8b7d4..084a2ae9d8 100644 --- a/src/test/java/com/microsoft/sqlserver/jdbc/SQLServerConnectionTest.java +++ b/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")) diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/connection/TimeoutTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/connection/TimeoutTest.java index ed9d90e270..68fbfc8dff 100644 --- a/src/test/java/com/microsoft/sqlserver/jdbc/connection/TimeoutTest.java +++ b/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. *