ctubbsii commented on code in PR #4380:
URL: https://github.com/apache/accumulo/pull/4380#discussion_r1692819552
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/DistributedReadWriteLock.java:
##########
@@ -179,15 +181,14 @@ public boolean tryLock() {
@Override
public boolean tryLock(long time, TimeUnit unit) throws
InterruptedException {
- long now = System.currentTimeMillis();
- long returnTime = now + MILLISECONDS.convert(time, unit);
- while (returnTime > now) {
+ Duration returnTime = Duration.of(time, unit.toChronoUnit());
+ NanoTime start = NanoTime.now();
+ while (start.elapsed().compareTo(returnTime) < 0) {
Review Comment:
Instead of using compareTo and `< 0`, which is a bit confusing, you could
add a `boolean hasElapsed(Duration)` method that checks if the amount of time
has elapsed yet or not. This could also be made more efficient internally by
not requiring a new Duration object to be constructed every time `elapsed()` is
called. Instead, just get the nanos for the supplied Duration and compare by
comparing the primitives.
##########
core/src/main/java/org/apache/accumulo/core/util/Retry.java:
##########
@@ -201,14 +202,14 @@ protected void sleep(Duration wait) throws
InterruptedException {
public void logRetry(Logger log, String message, Throwable t) {
// log the first time as debug, and then after every logInterval as a
warning
- long now = System.nanoTime();
+ NanoTime now = NanoTime.now();
if (hasNeverLogged) {
if (log.isDebugEnabled()) {
log.debug(getMessage(message, t));
}
hasNeverLogged = false;
lastRetryLog = now;
- } else if ((now - lastRetryLog) > logInterval.toNanos()) {
+ } else if (now.subtract(lastRetryLog).compareTo(logInterval) > 0) {
Review Comment:
This line is just as hard to understand before the change as it is after.
I think this is trying to express something like:
```java
} else if (lastRetryLog.hasElapsed(logInterval)) {
log.warn(getMessage(message), t);
lastRetryLog().reset();
hasLoggedWarn = true;
```
##########
core/src/main/java/org/apache/accumulo/core/util/OpTimer.java:
##########
@@ -20,17 +20,20 @@
import static java.util.concurrent.TimeUnit.NANOSECONDS;
+import java.time.Duration;
import java.util.concurrent.TimeUnit;
+import org.apache.accumulo.core.util.time.NanoTime;
+
/**
* Provides a stop watch for timing a single type of event. This code is based
on the
* org.apache.hadoop.util.StopWatch available in hadoop 2.7.0
*/
public class OpTimer {
private boolean isStarted;
- private long startNanos;
- private long currentElapsedNanos;
+ private NanoTime start;
+ private Duration currentElapsed = Duration.ZERO;
Review Comment:
Rather than change the implementation, it looks like this is another class
that tries to do the same kinds of things as the new NanoTime class. Their
functions should be merged, and we should have only one, not both. Instead of
changing this class, it'd be better to see if its uses can be replaced so it
can be deleted.
Related: the NanoTime class doesn't actually expose the nanosecond units
outside of the class. So, it doesn't need to be named "NanoTime" at all. It
could just be called "OpTimer" and replace this class, or called "Stopwatch" or
"ElapsedTime" or "Timer" or something else.
--
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]
