diff --git a/libs/cluster/Server/Gossip/GarnetServerNode.cs b/libs/cluster/Server/Gossip/GarnetServerNode.cs index cf1dcd49564..6c477e95b45 100644 --- a/libs/cluster/Server/Gossip/GarnetServerNode.cs +++ b/libs/cluster/Server/Gossip/GarnetServerNode.cs @@ -66,6 +66,9 @@ internal sealed class GarnetServerNode /// const int defaultMaxOutstandingTask = 8; + internal static int GetClientTimeoutMilliseconds(int clusterTimeoutSeconds) + => clusterTimeoutSeconds <= 0 ? 0 : (int)Math.Min((long)clusterTimeoutSeconds * 1000, int.MaxValue); + /// /// GarnetServerNode constructor /// @@ -83,7 +86,7 @@ public GarnetServerNode(ClusterProvider clusterProvider, EndPoint endpoint, SslC tlsOptions, sendPageSize: opts.DisablePubSub ? defaultSendPageSize : Math.Max(defaultSendPageSize, (int)opts.PubSubPageSizeBytes()), maxOutstandingTasks: defaultMaxOutstandingTask, - timeoutMilliseconds: opts.ClusterTimeout <= 0 ? 0 : TimeSpan.FromSeconds(opts.ClusterTimeout).Milliseconds, + timeoutMilliseconds: GetClientTimeoutMilliseconds(opts.ClusterTimeout), authUsername: clusterProvider.clusterManager.clusterProvider.ClusterUsername, authPassword: clusterProvider.clusterManager.clusterProvider.ClusterPassword, epoch: epoch, diff --git a/libs/server/Servers/GarnetServerOptions.cs b/libs/server/Servers/GarnetServerOptions.cs index f847c9d7447..45f5f41174d 100644 --- a/libs/server/Servers/GarnetServerOptions.cs +++ b/libs/server/Servers/GarnetServerOptions.cs @@ -261,7 +261,7 @@ public class GarnetServerOptions : ServerOptions /// /// Frequency (in seconds) of logging (used for tracking progress of long running operations e.g. migration) /// - public int LoggingFrequency = TimeSpan.FromSeconds(5).Seconds; + public int LoggingFrequency = 5; /// /// Metrics sampling frequency diff --git a/test/cluster/Garnet.test.cluster/GarnetServerNodeTimeoutTests.cs b/test/cluster/Garnet.test.cluster/GarnetServerNodeTimeoutTests.cs new file mode 100644 index 00000000000..ee07dd3c47c --- /dev/null +++ b/test/cluster/Garnet.test.cluster/GarnetServerNodeTimeoutTests.cs @@ -0,0 +1,47 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +using Garnet.cluster; +using NUnit.Framework; +using NUnit.Framework.Legacy; + +namespace Garnet.test.cluster +{ + /// + /// Regression tests for the gossip-connection client timeout computation + /// (), which previously used + /// TimeSpan.Milliseconds (sub-second component) instead of the full millisecond total. + /// + [TestFixture] + public class GarnetServerNodeTimeoutTests : TestBase + { + [Test] + [TestCase(1, 1_000)] + [TestCase(5, 5_000)] + [TestCase(60, 60_000)] // production default; buggy code yielded 0 here + [TestCase(3600, 3_600_000)] + public void ClientTimeoutUsesTotalMilliseconds(int clusterTimeoutSeconds, int expectedMilliseconds) + { + var actual = GarnetServerNode.GetClientTimeoutMilliseconds(clusterTimeoutSeconds); + ClassicAssert.AreEqual(expectedMilliseconds, actual); + } + + [Test] + [TestCase(0)] + [TestCase(-1)] + public void NonPositiveClusterTimeoutDisablesClientTimeout(int clusterTimeoutSeconds) + { + var actual = GarnetServerNode.GetClientTimeoutMilliseconds(clusterTimeoutSeconds); + ClassicAssert.AreEqual(0, actual); + } + + [Test] + [TestCase(2_147_484)] // *1000 overflows int; naive cast would wrap negative and disable the timeout + [TestCase(int.MaxValue)] + public void LargeClusterTimeoutClampsToIntMaxValue(int clusterTimeoutSeconds) + { + var actual = GarnetServerNode.GetClientTimeoutMilliseconds(clusterTimeoutSeconds); + ClassicAssert.AreEqual(int.MaxValue, actual); + } + } +} \ No newline at end of file diff --git a/test/standalone/Garnet.test.scripting/RespAofTests.cs b/test/standalone/Garnet.test.scripting/RespAofTests.cs index 34899e7e108..567c0fa13d9 100644 --- a/test/standalone/Garnet.test.scripting/RespAofTests.cs +++ b/test/standalone/Garnet.test.scripting/RespAofTests.cs @@ -333,7 +333,7 @@ public async Task AofExpiryRMWStoreRecoverTestAsync() // Verify 2nd string ttl var recoveredValueTtl = db.KeyTimeToLive("AofExpiryRMWStoreRecoverTestKey2"); ClassicAssert.IsTrue(recoveredValueTtl.HasValue); - ClassicAssert.Less(recoveredValueTtl.Value.Milliseconds, 8500); + ClassicAssert.LessOrEqual(recoveredValueTtl.Value.TotalMilliseconds, 10_000); ClassicAssert.Greater(recoveredValueTtl.Value.TotalSeconds, 0); } @@ -409,7 +409,7 @@ public async Task AofExpiryUpsertStoreRecoverTestAsync() // Verify 2nd string ttl var recoveredValueTtl = db.KeyTimeToLive("AofExpiryUpsertStoreRecoverTestKey2"); ClassicAssert.IsTrue(recoveredValueTtl.HasValue); - ClassicAssert.Less(recoveredValueTtl.Value.Milliseconds, 8500); + ClassicAssert.LessOrEqual(recoveredValueTtl.Value.TotalMilliseconds, 10_000); ClassicAssert.Greater(recoveredValueTtl.Value.TotalSeconds, 0); }