Skip to content

Commit

Permalink
Don't rely on test code execution time span.
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
lukas-vlcek committed Aug 13, 2024
1 parent b6c80b1 commit d1cc5c7
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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("Lag is not expected to be shorter than span [" + span + "ms]", 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("Lag is not expected to be shorter than span [" + randomSpan + "ms]", transferTracker.getTimeMsLag() >= randomSpan);
}

public void testAddUploadBytesStarted() {
Expand Down

0 comments on commit d1cc5c7

Please sign in to comment.