You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2024/08/09 22:56:18 UTC
[PR] adds a new count down timer [accumulo]
keith-turner opened a new pull request, #4796:
URL: https://github.com/apache/accumulo/pull/4796
Adds a class used to measure the amount of elapsed
time left from an initial duration.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds a new count down timer [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #4796:
URL: https://github.com/apache/accumulo/pull/4796#discussion_r1723837279
##########
core/src/test/java/org/apache/accumulo/core/util/CountDownTimerTest.java:
##########
@@ -30,29 +31,61 @@
public class CountDownTimerTest {
@Test
public void testCountDownTimer() throws Exception {
+
+ long start = System.nanoTime();
+
var timer1 = CountDownTimer.startNew(Duration.ofMillis(100));
Thread.sleep(10);
- var timer2 = CountDownTimer.startNew(Duration.ofMillis(100));
+ var timer2 = CountDownTimer.startNew(100, TimeUnit.MILLISECONDS);
Thread.sleep(10);
var timer3 = CountDownTimer.startNew(Duration.ofMillis(100));
Thread.sleep(10);
+ boolean expired1 = timer1.isExpired();
+ boolean expired2 = timer1.isExpired();
+ boolean expired3 = timer1.isExpired();
+
var left3 = timer3.timeLeft(TimeUnit.MILLISECONDS);
var left2 = timer2.timeLeft(TimeUnit.MILLISECONDS);
var left1 = timer1.timeLeft(TimeUnit.MILLISECONDS);
+ long elapsed = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start);
+
assertTrue(left3 <= 90);
assertTrue(left2 <= 80);
assertTrue(left1 <= 70);
- assertTrue(left3 >= left2);
- assertTrue(left2 >= left1);
+ assertTrue(Math.max(left3 - 10, 0) >= left2);
+ assertTrue(Math.max(left2 - 10, 0) >= left1);
+ assertTrue(left1 >= 100 - elapsed);
Review Comment:
I am getting this test to fail locally too pretty consistently.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds a new count down timer [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #4796:
URL: https://github.com/apache/accumulo/pull/4796#issuecomment-2299072741
@DomGarguilo added the suggested methods in ce790d6
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds a new count down timer [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #4796:
URL: https://github.com/apache/accumulo/pull/4796#discussion_r1723819163
##########
core/src/test/java/org/apache/accumulo/core/util/CountDownTimerTest.java:
##########
@@ -30,29 +31,61 @@
public class CountDownTimerTest {
@Test
public void testCountDownTimer() throws Exception {
+
+ long start = System.nanoTime();
+
var timer1 = CountDownTimer.startNew(Duration.ofMillis(100));
Thread.sleep(10);
- var timer2 = CountDownTimer.startNew(Duration.ofMillis(100));
+ var timer2 = CountDownTimer.startNew(100, TimeUnit.MILLISECONDS);
Thread.sleep(10);
var timer3 = CountDownTimer.startNew(Duration.ofMillis(100));
Thread.sleep(10);
+ boolean expired1 = timer1.isExpired();
+ boolean expired2 = timer1.isExpired();
+ boolean expired3 = timer1.isExpired();
+
var left3 = timer3.timeLeft(TimeUnit.MILLISECONDS);
var left2 = timer2.timeLeft(TimeUnit.MILLISECONDS);
var left1 = timer1.timeLeft(TimeUnit.MILLISECONDS);
+ long elapsed = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start);
+
assertTrue(left3 <= 90);
assertTrue(left2 <= 80);
assertTrue(left1 <= 70);
- assertTrue(left3 >= left2);
- assertTrue(left2 >= left1);
+ assertTrue(Math.max(left3 - 10, 0) >= left2);
+ assertTrue(Math.max(left2 - 10, 0) >= left1);
+ assertTrue(left1 >= 100 - elapsed);
Review Comment:
I was working on some doc changes on a branch I made off of this one and saw this test fail here: https://github.com/DomGarguilo/accumulo/actions/runs/10477317343/job/29018210951#step:6:711
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds a new count down timer [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #4796:
URL: https://github.com/apache/accumulo/pull/4796#issuecomment-2283934477
This looks good to me so far. I have two suggestions:
1. Would it be beneficial to include both a `startNew(Duration duration)` method and a `startNew(long duration, TimeUnit units)` method?
2. Would it be useful to add a method that simply returns a boolean indicating whether the countdown timer has finished? This would allow us to check if the timer is done without needing to know the exact time remaining.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds a new count down timer [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4796:
URL: https://github.com/apache/accumulo/pull/4796#discussion_r1723924982
##########
core/src/test/java/org/apache/accumulo/core/util/CountDownTimerTest.java:
##########
@@ -30,29 +31,61 @@
public class CountDownTimerTest {
@Test
public void testCountDownTimer() throws Exception {
+
+ long start = System.nanoTime();
+
var timer1 = CountDownTimer.startNew(Duration.ofMillis(100));
Thread.sleep(10);
- var timer2 = CountDownTimer.startNew(Duration.ofMillis(100));
+ var timer2 = CountDownTimer.startNew(100, TimeUnit.MILLISECONDS);
Thread.sleep(10);
var timer3 = CountDownTimer.startNew(Duration.ofMillis(100));
Thread.sleep(10);
+ boolean expired1 = timer1.isExpired();
+ boolean expired2 = timer1.isExpired();
+ boolean expired3 = timer1.isExpired();
+
var left3 = timer3.timeLeft(TimeUnit.MILLISECONDS);
var left2 = timer2.timeLeft(TimeUnit.MILLISECONDS);
var left1 = timer1.timeLeft(TimeUnit.MILLISECONDS);
+ long elapsed = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start);
+
assertTrue(left3 <= 90);
assertTrue(left2 <= 80);
assertTrue(left1 <= 70);
- assertTrue(left3 >= left2);
- assertTrue(left2 >= left1);
+ assertTrue(Math.max(left3 - 10, 0) >= left2);
+ assertTrue(Math.max(left2 - 10, 0) >= left1);
+ assertTrue(left1 >= 100 - elapsed);
Review Comment:
That was interesting, was passing locally when I ran it a few times. Ran it a bunch of times and it failed. I think the problem was that inside CountDownTimer it subtracts two nanos and then convers to millis. In the test code it was converting to millis and then subtracting. I suspect the different order lead to a loss of precision, the order CountDownTimer is doing the operations is more precise and the way the test was doing things was less precise.
I converted the test code to use Timer so that I could get a duration. I did subtraction using duration which should make the test more precise. Made a lot of changes in b13f932 but
this snippet `Duration.ofMillis(100).minus(elapsed).toMillis()` should fix the problem you saw. Let me know if still seeing the problem.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] adds a new count down timer [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #4796:
URL: https://github.com/apache/accumulo/pull/4796
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org