From 510c03aa143dd745043460c4aa4ec530f2ccf1ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vl=C4=8Dek?= Date: Fri, 9 Aug 2024 20:33:01 +0200 Subject: [PATCH] Don't rely on test code execution time span. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Current implementation of [`RemoteSegmentTransferTrackerTests.testComputeTimeLagOnUpdate()`](https://github.com/opensearch-project/OpenSearch/blob/2b17902643738f0d2a75ade7c85cbca94d18ce49/server/src/test/java/org/opensearch/index/remote/RemoteSegmentTransferTrackerTests.java#L139) test rely on some assumptions about how fast the testing code will finish in JVM. Moreover it does not precisely control boundaries of the time span, specifically the start of the span because it is determined by internal implementation of [`RemoteSegmentTransferTracker.getTimeMsLag()`](https://github.com/opensearch-project/OpenSearch/blob/2b17902643738f0d2a75ade7c85cbca94d18ce49/server/src/main/java/org/opensearch/index/remote/RemoteSegmentTransferTracker.java#L262) which indirectly makes call to `System.nanoTime()`. This commit loosens the assumption that the test code execution will finish within +/-20ms. Instead it only assumes that the execution time span won't be shorter than predefined (and controlled) thread sleep interval and any larger interval value is considered a success. The whole point of this test is not to verify execution speed with defined precision. Instead the point is that the [`getTimeMsLag()`](https://github.com/opensearch-project/OpenSearch/blob/2b17902643738f0d2a75ade7c85cbca94d18ce49/server/src/main/java/org/opensearch/index/remote/RemoteSegmentTransferTracker.java#L262) method returns either 0 (for specific conditions) or possitive number (assuming that `remoteRefreshStartTimeMs` is not greater than `System.nanoTime()`). Closes: #14325 Signed-off-by: Lukáš Vlček --- .../index/remote/RemoteSegmentTransferTracker.java | 4 ++-- .../remote/RemoteSegmentTransferTrackerTests.java | 13 +++++++------ 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteSegmentTransferTracker.java b/server/src/main/java/org/opensearch/index/remote/RemoteSegmentTransferTracker.java index f1843ea3eef38..a29bd1d840b43 100644 --- a/server/src/main/java/org/opensearch/index/remote/RemoteSegmentTransferTracker.java +++ b/server/src/main/java/org/opensearch/index/remote/RemoteSegmentTransferTracker.java @@ -65,7 +65,7 @@ public class RemoteSegmentTransferTracker extends RemoteTransferTracker { private volatile long remoteRefreshSeqNo; /** - * The refresh time of most recent remote refresh. + * The refresh time of the most recent remote refresh. */ private volatile long remoteRefreshTimeMs; @@ -76,7 +76,7 @@ public class RemoteSegmentTransferTracker extends RemoteTransferTracker { private volatile long remoteRefreshStartTimeMs = -1; /** - * The refresh time(clock) of most recent remote refresh. + * The refresh time(clock) of the most recent remote refresh. */ private volatile long remoteRefreshClockTimeMs; diff --git a/server/src/test/java/org/opensearch/index/remote/RemoteSegmentTransferTrackerTests.java b/server/src/test/java/org/opensearch/index/remote/RemoteSegmentTransferTrackerTests.java index 1ec1e9977a9d5..38a6066ee5075 100644 --- a/server/src/test/java/org/opensearch/index/remote/RemoteSegmentTransferTrackerTests.java +++ b/server/src/test/java/org/opensearch/index/remote/RemoteSegmentTransferTrackerTests.java @@ -152,15 +152,16 @@ public void testComputeTimeLagOnUpdate() throws InterruptedException { transferTracker.updateLocalRefreshTimeMs(currentTimeMsUsingSystemNanos()); transferTracker.updateLatestLocalFileNameLengthMap(List.of("test"), k -> 1L); - // Sleep for 100ms and then the lag should be within 100ms +/- 20ms - Thread.sleep(100); - assertTrue(Math.abs(transferTracker.getTimeMsLag() - 100) <= 20); + // Sleep for 100ms and then the lag should not be shorter + long span = 100; + Thread.sleep(span); + assertTrue(transferTracker.getTimeMsLag() >= span); transferTracker.updateRemoteRefreshTimeMs(transferTracker.getLocalRefreshTimeMs()); transferTracker.updateLocalRefreshTimeMs(currentTimeMsUsingSystemNanos()); - long random = randomIntBetween(50, 200); - Thread.sleep(random); - assertTrue(Math.abs(transferTracker.getTimeMsLag() - random) <= 20); + long randomSpan = randomIntBetween(50, 200); + Thread.sleep(randomSpan); + assertTrue(transferTracker.getTimeMsLag() >= randomSpan); } public void testAddUploadBytesStarted() {