You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2022/06/07 23:07:17 UTC
[GitHub] [geode] nabarunnag opened a new pull request, #7779: GEODE-6504: Instant class used.
nabarunnag opened a new pull request, #7779:
URL: https://github.com/apache/geode/pull/7779
* Using instant class instead of nanos.
* Nanos is not updated that often hence not proper resolution.
* Using millis instead of seconds to avoid ceiling issues.
<!-- Thank you for submitting a contribution to Apache Geode. -->
<!-- In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
-->
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
<!-- Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
-->
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7779: GEODE-6504: Instant class used.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #7779:
URL: https://github.com/apache/geode/pull/7779#discussion_r893817487
##########
geode-core/src/integrationTest/java/org/apache/geode/cache/RegionExpirationIntegrationTest.java:
##########
@@ -84,15 +85,16 @@ public void increaseRegionTtl() throws Exception {
.setRegionTimeToLive(new ExpirationAttributes(secondTtlSeconds, DESTROY));
await().until(region::isDestroyed);
- assertThat(NANOSECONDS.toSeconds(nanoTime() - startNanos))
- .isGreaterThanOrEqualTo(secondTtlSeconds);
+ Instant endInstant = Instant.now();
+ assertThat(Duration.between(startInstant, endInstant))
+ .isGreaterThanOrEqualTo(Duration.ofSeconds(secondTtlSeconds));
}
@Test
- public void decreaseRegionTtl() throws Exception {
+ public void decreaseRegionTtl() {
int firstTtlSeconds = 5;
Review Comment:
For readability I wonder if this is clearer now that we have introduced these new time classes:
```java
final Duration firstTtl = Duration.ofSeconds(5);
final Duration secondTtl = Duration.ofSeconds(1);
...
regionFactory.setRegionTimeToLive(new ExpirationAttributes(firstTtl.getSeconds(), DESTROY));
...
assertThat(Duration.between(startInstant, endInstant))
.isLessThan(firstTtlSeconds);
```
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nabarunnag commented on a diff in pull request #7779: GEODE-6504: Instant class used.
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on code in PR #7779:
URL: https://github.com/apache/geode/pull/7779#discussion_r893014322
##########
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:
Instant class, newly introduced in Java 8. If it is the same as currentTimeMillis(), should not matter which one we use right?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7779: GEODE-6504: Instant class used.
Posted by GitBox <gi...@apache.org>.
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nabarunnag commented on a diff in pull request #7779: GEODE-6504: Instant class used.
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on code in PR #7779:
URL: https://github.com/apache/geode/pull/7779#discussion_r893014322
##########
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:
Instant class, newly introduced in Java 8. If both are the same, should not matter which one we use right?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nabarunnag commented on a diff in pull request #7779: GEODE-6504: Instant class used.
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on code in PR #7779:
URL: https://github.com/apache/geode/pull/7779#discussion_r893746594
##########
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:
- I would prefer to see in this test that it is greater or equal to the increased TTL. (lets try this, stress tests were ok, if it fails again I will make the change)
- Using the Duration comparison
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nabarunnag commented on a diff in pull request #7779: GEODE-6504: Instant class used.
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on code in PR #7779:
URL: https://github.com/apache/geode/pull/7779#discussion_r893011483
##########
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:
its in the javadocs and lot of blogs:
> Another pitfall with nanoTime() is that even though it provides nanosecond precision, it doesn't guarantee nanosecond resolution (i.e. how often the value is updated).
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nabarunnag commented on a diff in pull request #7779: GEODE-6504: Instant class used.
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on code in PR #7779:
URL: https://github.com/apache/geode/pull/7779#discussion_r893017368
##########
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:
In the history of the this test failure if it always fails with: expected 8 sec and actual 7 sec
even if the nanoTIme is off by 1ns the difference is 7999999999 which when is passed to NANOSECOND.toSecond() just divides it by 1000000000 which is 7 sec and the test fails.
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nabarunnag merged pull request #7779: GEODE-6504: Instant class used.
Posted by GitBox <gi...@apache.org>.
nabarunnag merged PR #7779:
URL: https://github.com/apache/geode/pull/7779
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] pivotal-jbarrett commented on a diff in pull request #7779: GEODE-6504: Instant class used.
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on code in PR #7779:
URL: https://github.com/apache/geode/pull/7779#discussion_r892974555
##########
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:
I don't understand the change based on the commit message. How is `nanoTime()` a lower resolution clock that `currentTimeMilis()`, used to construct this `Instant`?
--
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: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org