Don't rely on test code execution time span for RemoteSegmentTransferTrackerTests

[lukas-vlcek]

Description

Current implementation of RemoteSegmentTransferTrackerTests.testComputeTimeLagOnUpdate() 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() 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() method returns either 0 (for specific conditions) or possitive number (assuming that remoteRefreshStartTimeMs is not greater than System.nanoTime()).

Related Issues

Closes: #14325

Check List

• Functionality includes testing.
• [ ] API changes companion pull request created, if applicable.
• [ ] Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[lukas-vlcek]

Please add skip-changelog label.

[lukas-vlcek]

Idea for possible future improvement:

We could remove all direct calls to System.nanoTime() and similar System time methods from RemoteSegmentTransferTracker.java class and delegate it to some "TimeProvider" object. Then we could implement tests that have some more specific assumptions about code execution time span, because we would be able to control the TimeProvider in the test precisely.

If there is an agreement that this would be beneficial then we can open a new ticket.

[github-actions]

✅ Gradle check result for 025c303: SUCCESS

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.92%. Comparing base (b6c80b1) to head (6e191a3).
Report is 6 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #15187      +/-   ##
============================================
+ Coverage     71.90%   71.92%   +0.01%     
- Complexity    63033    63114      +81     
============================================
  Files          5197     5197              
  Lines        295313   295313              
  Branches      42677    42677              
============================================
+ Hits         212354   212390      +36     
- Misses        65552    65607      +55     
+ Partials      17407    17316      -91     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[linuxpi]

Thanks for raising a fix for this flaky test @lukas-vlcek, seems like a hot one.

Idea for possible future improvement:

We could remove all direct calls to System.nanoTime() and similar System time methods from RemoteSegmentTransferTracker.java class and delegate it to some "TimeProvider" object. Then we could implement tests that have some more specific assumptions about code execution time span, because we would be able to control the TimeProvider in the test precisely.

If there is an agreement that this would be beneficial then we can open a new ticket.

As of now we have always relied of directly using the System.nanoTime() but i think it would be good to have an abstraction like TimeProvider . To get an agreement we can maybe open a small RFC and get some thoughts from others

[github-actions]

❌ Gradle check result for 510c03a: UNSTABLE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 18dbf31: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❕ Gradle check result for d1cc5c7: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[linuxpi]

Mostly changes look good. One thing is we should run multiple iterations locally to make sure we have not regressed

[lukas-vlcek]

@linuxpi As for TimeProvider RFC, I will check other uses of System.nanoTime() in the code base (once I'm back from 🌴) and I will let you know what I think.

[github-actions]

❕ Gradle check result for 6e191a3: UNSTABLE

• TEST FAILURES:

      1 org.opensearch.index.ShardIndexingPressureIT.testShardIndexingPressureTrackingDuringBulkWrites

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.