You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "DomGarguilo (via GitHub)" <gi...@apache.org> on 2024/03/11 15:29:03 UTC
[PR] Replace long + TimeUnit with Duration in ReadOnlyTStore.unreserve() [accumulo]
DomGarguilo opened a new pull request, #4358:
URL: https://github.com/apache/accumulo/pull/4358
This PR replaces the use of a long and a TimeUnit with a Duration object. The motivation behind this change is to use the Duration object to simplify and make usage more evident.
--
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] Replace long + TimeUnit with Duration in ReadOnlyTStore.unreserve() [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4358:
URL: https://github.com/apache/accumulo/pull/4358#discussion_r1520026952
##########
core/src/main/java/org/apache/accumulo/core/fate/ZooStore.java:
##########
@@ -63,7 +63,7 @@ public class ZooStore<T> implements TStore<T> {
private ZooReaderWriter zk;
private String lastReserved = "";
private Set<Long> reserved;
- private Map<Long,Long> deferred;
+ private Map<Long,Duration> deferred;
Review Comment:
For correctness this probably needs to stay as Long
```suggestion
private Map<Long,Long> deferred;
```
This is because the impl of duration has a long seconds and int nanoseconds internally. Therefore the following program
```java
long t1 = Long.MAX_VALUE - 30;
long t2 = t1+40;
System.out.println(t2-t1);
Duration d1 = Duration.ofNanos(t1);
Duration d2 = Duration.ofNanos(t2);
System.out.println(d2.minus(d1));
```
prints
```
40
PT-5124095H-34M-33.709551576S
```
which is incorrect.
The way System.nanoTime is intended to be used it relies on the behavior of subtracting longs that cross the max long boundary. Java does not provide any good high level classes for dealing with this nuanced behavior AFAIK. There is also a java.time.Instant class that seems like it could be useful in this situation as the value for the map. Having looked into it in the past, found Instant was also not good to use w/ System.nanoTime, but I can not remember the specifics.
This class can accept Duration as a method argument, it just needs to use Long internally. I would recommend sticking w/ long and adding some comments about the dangers of using 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] Replace long + TimeUnit with Duration in ReadOnlyTStore.unreserve() [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4358:
URL: https://github.com/apache/accumulo/pull/4358#discussion_r1520026952
##########
core/src/main/java/org/apache/accumulo/core/fate/ZooStore.java:
##########
@@ -63,7 +63,7 @@ public class ZooStore<T> implements TStore<T> {
private ZooReaderWriter zk;
private String lastReserved = "";
private Set<Long> reserved;
- private Map<Long,Long> deferred;
+ private Map<Long,Duration> deferred;
Review Comment:
For correctness this probably needs to stay as Long
```suggestion
private Map<Long,Long> deferred;
```
This is because the impl of duration has a long seconds and int nanoseconds internally. Therefore the following program
```java
long t1 = Long.MAX_VALUE - 30;
long t2 = t1+40;
System.out.println(t2-t1);
Duration d1 = Duration.ofNanos(t1);
Duration d2 = Duration.ofNanos(t2);
System.out.println(d2.minus(d1));
```
prints
```
40
PT-5124095H-34M-33.709551576S
```
which is incorrect.
The way System.nanoTime is intended to be used it relies on the behavior of subtracting longs that cross the max long boundary. Java does not provide any good high level classes for dealing with this nuanced behavior AFAIK. There is also a java.time.Instant class that seems like it could be useful in this situation as the value for the map. Having looked into it in the past, found Instance was also not good to use w/ System.nanoTime, but I can not remember the specifics.
This class can accept Duration as a method argument, it just needs to use Long internally. I would recommend sticking w/ long and adding some comments about the dangers of using 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] Replace long + TimeUnit with Duration in ReadOnlyTStore.unreserve() [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #4358:
URL: https://github.com/apache/accumulo/pull/4358#issuecomment-1989346735
Using nanoTime is more correct than currentTimeMillis, but using nanoTime correctly is tricky. Thinking we may benefit from having an internal class that wraps nanoTime and limits what can be done with it to operations that are known to work well with nanoTime. Maybe something like the following.
```java
package org.apache.accumulo.core.util;
import java.time.Duration;
public class NanoInstant {
private final long nanoTime;
private NanoInstant(long nanoTime) {
this.nanoTime = nanoTime;
}
public NanoInstant add(Duration duration) {
return new NanoInstant(nanoTime+duration.toNanos());
}
// elapsed it the terminology used in the nanoTime javadoc
public Duration elapsed(NanoInstant nanoInstant) {
return Duration.ofNanos(nanoTime - nanoInstant.nanoTime);
}
public static NanoInstant now(){
return new NanoInstant(System.nanoTime());
}
}
```
--
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] Replace long + TimeUnit with Duration in ReadOnlyTStore.unreserve() [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #4358:
URL: https://github.com/apache/accumulo/pull/4358#discussion_r1520308778
##########
core/src/main/java/org/apache/accumulo/core/fate/ZooStore.java:
##########
@@ -202,12 +202,10 @@ public long reserve() {
if (deferred.isEmpty()) {
this.wait(5000);
} else {
- long currTime = System.nanoTime();
- long minWait =
- deferred.values().stream().mapToLong(l -> l - currTime).min().getAsLong();
- long waitTime = TimeUnit.MILLISECONDS.convert(minWait, TimeUnit.NANOSECONDS);
- if (waitTime > 0) {
- this.wait(Math.min(waitTime, 5000));
+ final long now = System.nanoTime();
+ long minWait = deferred.values().stream().mapToLong(l -> l - now).min().orElseThrow();
+ if (minWait > 0) {
Review Comment:
Good catch. Fixed in 55e1c1e.
--
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] Replace long + TimeUnit with Duration in ReadOnlyTStore.unreserve() [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4358:
URL: https://github.com/apache/accumulo/pull/4358#discussion_r1520026952
##########
core/src/main/java/org/apache/accumulo/core/fate/ZooStore.java:
##########
@@ -63,7 +63,7 @@ public class ZooStore<T> implements TStore<T> {
private ZooReaderWriter zk;
private String lastReserved = "";
private Set<Long> reserved;
- private Map<Long,Long> deferred;
+ private Map<Long,Duration> deferred;
Review Comment:
For correctness this probably needs to stay as Long
```suggestion
private Map<Long,Long> deferred;
```
This is because the impl of duration has a long seconds and int nanoseconds internally. Therefore the following program
```java
long t1 = Long.MAX_VALUE - 30;
long t2 = t1+40;
System.out.println(t2-t1);
Duration d1 = Duration.ofNanos(t1);
Duration d2 = Duration.ofNanos(t2);
System.out.println(d2.minus(d1));
```
prints
```
40
PT-5124095H-34M-33.709551576S
```
which is incorrect.
The way System.nanoTime is intended to be used it relies on the behavior of subtracting longs that cross the max long boundary. Java does not provide any good high level classes for dealing with nuanced behavior AFAIK. There is also a java.time.Instant class that seems like it could be useful in this situation as the value for the map. Having looked into it in the past, found Instance was also not good to use w/ System.nanoTime, but I can not remember the specifics.
This class can accept Duration as a method argument, it just needs to use Long internally. I would recommend sticking w/ long and adding some comments about the dangers of using 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] Replace long + TimeUnit with Duration in ReadOnlyTStore.unreserve() [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4358:
URL: https://github.com/apache/accumulo/pull/4358#discussion_r1520275733
##########
core/src/main/java/org/apache/accumulo/core/fate/ZooStore.java:
##########
@@ -202,12 +202,10 @@ public long reserve() {
if (deferred.isEmpty()) {
this.wait(5000);
} else {
- long currTime = System.nanoTime();
- long minWait =
- deferred.values().stream().mapToLong(l -> l - currTime).min().getAsLong();
- long waitTime = TimeUnit.MILLISECONDS.convert(minWait, TimeUnit.NANOSECONDS);
- if (waitTime > 0) {
- this.wait(Math.min(waitTime, 5000));
+ final long now = System.nanoTime();
+ long minWait = deferred.values().stream().mapToLong(l -> l - now).min().orElseThrow();
+ if (minWait > 0) {
Review Comment:
This code was previously converting nanos to millis. Seems like that is still needed
--
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] Replace long + TimeUnit with Duration in ReadOnlyTStore.unreserve() [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo merged PR #4358:
URL: https://github.com/apache/accumulo/pull/4358
--
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