Repository navigation
Conversation
|
|
|
Welcome @flrnd! |
|
/lgtm |
|
The same failure occurs on Java 11, 17, and 21 in the Windows Bazel build. In this case, I now wonder if it is worth finding a deterministic way to generate the timestamps, or if leaving the sleep in place makes sense. |
|
Another alternative to @@ -144,8 +138,8 @@ class EventCorrelatorTest {
Boolean expectedSkip)
throws Exception {
EventCorrelator correlator = new EventCorrelator();
+ OffsetDateTime now = OffsetDateTime.now();
for (CoreV1Event event : previousEvents) {
- OffsetDateTime now = OffsetDateTime.now();
event.setFirstTimestamp(now);
event.setLastTimestamp(now);
Optional<MutablePair<CoreV1Event, V1Patch>> result = correlator.correlate(event);
@@ -153,7 +147,7 @@ class EventCorrelatorTest {
correlator.updateState(event);
}
}
- OffsetDateTime now = OffsetDateTime.now();
+ now = now.plusNanos(10);
newEvent.setFirstTimestamp(now);
newEvent.setLastTimestamp(now); |
|
Feel free to try that alternate fix and update this PR. |
bd1ce4a to
9f9dcc7
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, flrnd The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
There are now test failures in this test in some CI/CD runs. |
Avoid relying on Thread.sleep(100) to produce distinct timestamps. The sleep was removed because the test only requires timestamp ordering, not elapsed time. However, the Windows Bazel build showed that OffsetDateTime.now() can return identical timestamps in consecutive calls depending on the environment clock resolution. Advance the timestamp explicitly instead, keeping the test deterministic across different platforms.
9f9dcc7 to
95b4da2
Compare
|
New changes are detected. LGTM label has been removed. |
Notes
Replace
Thread.sleep(100)with an explicit timestamp increment. The test only requires distinct timestamps to verify event compression, not actual elapsed time.CI failures
The initial change used
plusNanos(10)to separate the timestamps. While the tests passed on Linux and macOS across JDK 11, 17, and 21, and on Windows with JDK 11, CI reported failures on Windows with JDK 17 and 21.The failures were not consistently reproducible locally, even when using a comparable environment:
--config ciInvestigation
To investigate, I instrumented
EventCorrelatorTestandEventAggregatorto compare the generated timestamps, test inputs, and correlated outputs.The observations were:
plusNanos(0), the duplicate-event cases (counts 2 and 11) failed because the correlated events had identicalfirstTimestampandlastTimestampvalues.plusNanos(1000)(1 μs), both cases passed, and the correlated outputs preserved the expected timestamp difference.These results confirm that explicitly separating the input timestamps addresses the observed assertion failure. However, they do not fully explain why the original 10 ns increment failed intermittently in CI.
Final adjustment
Increase the increment to 1 ms using:
This provides a larger, explicit separation between timestamps while keeping the test independent of execution timing.
The tests pass locally with Eclipse Temurin JDK 11, 17, and 21 on Windows using Bazel's CI configuration.
This removes the reliance on
Thread.sleep(100)and avoids depending on consecutive calls to the system clock to produce distinct timestamps.