DomGarguilo commented on code in PR #4866:
URL: https://github.com/apache/accumulo/pull/4866#discussion_r1750401448


##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftScanner.java:
##########
@@ -343,14 +343,12 @@ public static List<KeyValue> scan(ClientContext context, 
ScanState scanState, lo
         if (Thread.currentThread().isInterrupted()) {
           throw new AccumuloException("Thread interrupted");
         }
-
-        if ((System.currentTimeMillis() - startTime) / 1000.0 > timeOut) {
+        if (startTime.elapsed(MILLISECONDS) > timeOut.toMillis()) {

Review Comment:
   ```suggestion
           if (scanTimer.hasElapsed(timeOut)) {
   ```
   
   Same thing here and the if statement a few lines down. Can be simplified 
using `hasElapsed()` and potentially be made more clear by renaming the var to 
`scanTimer` or something like that.



##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftScanner.java:
##########
@@ -275,12 +275,12 @@ static <T> Optional<T> waitUntil(Supplier<Optional<T>> 
condition, Duration maxWa
         .incrementBy(100, MILLISECONDS).maxWait(1, SECONDS).backOffFactor(1.5)
         .logInterval(3, TimeUnit.MINUTES).createRetry();
 
-    long startTime = System.nanoTime();
+    Timer startTime = Timer.startNew();
     Optional<T> optional = condition.get();
     while (optional.isEmpty()) {
       log.trace("For tableId {} scan server selector is waiting for '{}'", 
tableId, description);
 
-      var elapsedTime = Duration.ofNanos(System.nanoTime() - startTime);
+      var elapsedTime = startTime.elapsed();
 
       if (elapsedTime.compareTo(timeoutLeft) > 0) {

Review Comment:
   ```suggestion
         if (startTime.hasElapsed(timeoutLeft)) {
   ```
   
   There are a few places where `Timer.hasElapsed()` can be used instead of 
needing to grab the elapsed time via  `elapsed()` and then `compareTo()`.
   
   I also think it makes the code read better if the vars are renamed to timer. 
In this case for example `startTime.hasElapsed()` gives off the wrong idea 
while reading it (in my head at least) vs. something like 
`waitTimer.hasElapsed()`.



-- 
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]

Reply via email to