pivotal-jbarrett commented on code in PR #7779:
URL: https://github.com/apache/geode/pull/7779#discussion_r893607561
##########
geode-core/src/integrationTest/java/org/apache/geode/cache/RegionExpirationIntegrationTest.java:
##########
@@ -70,10 +71,10 @@ public void setUp() {
}
@Test
- public void increaseRegionTtl() throws Exception {
+ public void increaseRegionTtl() {
int firstTtlSeconds = 3;
int secondTtlSeconds = 8;
- long startNanos = nanoTime();
+ Instant startInstant = Instant.now();
Review Comment:
It sounds to me that it is more of a math problem, not a time resolution
problem. While `Instant` is meant for very precise instance in time, typically
for wall time, the `now()` construction of time is not very precise and uses
the wall clock from `System.currentTimeMillis()`. Using a lower resolution
timer may reduce then probably of the unit conversion floor but doesn't
eliminate it.
This assertion still fails:
```java
assertThat(TimeUnit.MILLISECONDS.toSeconds(7999L)).isEqualTo(8);
```
The problem comes form the conversion of time from higher resolution
duration to lower resolution duration and the expectation that this operation
happens at exactly the same time resolution as the measurement of the passage
of time. Compounding the change is that use of wall time from `Instant` could
actually be skewed by clock adjustments, where as `nanoTime` is a relatively
"fixed clock"*.
Basically, we can't say that this operation happened at precisely
`8.000000000s` or even `8.000s`. We could assert that it took place
approximately 8s with some tolerance. If we know that the event timer of the
operation we are measuring has a resolution finer than 1s, and that an event
never fires early, then we could assert that it took at least 7s.
My guess is, without digging Ito the internals, that the event time
resolution is 1ms based on `System.currentTimeMillis()` so the use of `Instant`
will "work" because they both use the same clock. My concern with the use of
`Instant` is more a matter of consistency with the rest of our tests and that
the source clock is somewhat obscured. If we are going to use `Instant` and
`Duration`, then we should probably use the `assertThat()` assertions for
`Duration`, like `closeTo()`.
this:
```java
assertThat(Duration.between(startInstant, endInstant))
.isGreaterThanOrEqualTo(Duration.ofSeconds(secondTtlSeconds));
```
or this:
```java
assertThat(Duration.between(startInstant, endInstant))
.isCloseTo(Duration.ofSeconds(secondTtlSeconds), Duration.ofSeconds(1));
```
_* `nanoTime()` has issues with different observers seeing different values
for the current tick leading some observations that time has either stopped or
gone backwards._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
