You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/01/30 14:25:17 UTC
[GitHub] [accumulo] cradal opened a new pull request #1489: Fixes #1458 -
ZooSession now uses nanoTime vice currentTimeMillis
cradal opened a new pull request #1489: Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
URL: https://github.com/apache/accumulo/pull/1489
Updated class to use nanoTime() as the base start time for duration calculations.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii merged pull request #1489: Fixes #1458 -
ZooSession now uses nanoTime vice currentTimeMillis
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #1489: Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
URL: https://github.com/apache/accumulo/pull/1489
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on issue #1489: Fixes #1458 -
ZooSession now uses nanoTime vice currentTimeMillis
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1489: Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
URL: https://github.com/apache/accumulo/pull/1489#issuecomment-584789584
I think the failures you're talking about with the ITs can happen with or without this change, and would consider that a separate issue.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1489:
Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1489: Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
URL: https://github.com/apache/accumulo/pull/1489#discussion_r373021289
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java
##########
@@ -119,8 +119,19 @@ public static ZooKeeper connect(String host, int timeout, String scheme, byte[]
boolean tryAgain = true;
long sleepTime = 100;
ZooKeeper zooKeeper = null;
-
- long startTime = System.currentTimeMillis();
+ /*
+ * Originally, startTime = System.currentTimeMillis(). Changed to System.nanoTime() to more
+ * accurately compute durations because it is not based on system clock variations. The
+ * ZooKeeper method signature expects an int value for 'timeout' and performs several
+ * calculations that can result in Numeric Expression Overflow errors if large, nanosecond
+ * values are used. For ths reason, System.nanoTime() is is converted to MS units prior to being
+ * used in calculations. Although, the resolution of 'startTime' is still in the MS range, the
+ * value from which it is calculated is nanoTime. Also, the MS units are preserved in the
+ * original method code, without the need for conversion. TimeUnit.convert is not used because
+ * "conversions from fine to coarser granularities truncate, so lose precision" (Oracle Java 7
+ * API)
+ */
Review comment:
I don't think any of this is necessary to keep in the code, since it is explaining why the code was changed, and not explaining the code that is there at the end.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] cradal commented on a change in pull request #1489:
Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1489: Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
URL: https://github.com/apache/accumulo/pull/1489#discussion_r373485314
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java
##########
@@ -119,8 +119,19 @@ public static ZooKeeper connect(String host, int timeout, String scheme, byte[]
boolean tryAgain = true;
long sleepTime = 100;
ZooKeeper zooKeeper = null;
-
- long startTime = System.currentTimeMillis();
+ /*
+ * Originally, startTime = System.currentTimeMillis(). Changed to System.nanoTime() to more
+ * accurately compute durations because it is not based on system clock variations. The
+ * ZooKeeper method signature expects an int value for 'timeout' and performs several
+ * calculations that can result in Numeric Expression Overflow errors if large, nanosecond
+ * values are used. For ths reason, System.nanoTime() is is converted to MS units prior to being
+ * used in calculations. Although, the resolution of 'startTime' is still in the MS range, the
+ * value from which it is calculated is nanoTime. Also, the MS units are preserved in the
+ * original method code, without the need for conversion. TimeUnit.convert is not used because
+ * "conversions from fine to coarser granularities truncate, so lose precision" (Oracle Java 7
+ * API)
+ */
+ long startTime = System.nanoTime() / 1000000;
Review comment:
Indeed, that makes more sense. Working the issue along those lines, there are two sections in ZooSession where calculations are performed using millis values for all the operands:
```java
if (System.currentTimeMillis() - startTime > 2L * timeout) {
throw new RuntimeException("Failed to connect to zookeeper (" + host
+ ") within 2x zookeeper timeout period " + timeout);
}
```
```java
if (tryAgain) {
if (startTime + 2L * timeout < System.currentTimeMillis() + sleepTime + connectTimeWait)
sleepTime = startTime + 2L * timeout - System.currentTimeMillis() - connectTimeWait;
if (sleepTime < 0) {
connectTimeWait -= sleepTime;
sleepTime = 0;
}
UtilWaitThread.sleep(sleepTime);
if (sleepTime < 10000)
sleepTime = sleepTime + (long) (sleepTime * secureRandom.nextDouble());
}
```
One solution is to convert from millis to nano when the values are first assigned. There are a couple issues I see with doing that. One, timeout is still being passed into the method in millis and would need to be converted at the point-of-use (or assigned a value within the method, which breaks convention). Two, there are calculations that use these values, but do not use startTime, that are already working correctly. Also, TIME_BETWEEN_CONNECT_CHECKS_MS would likely need to be refactored to ...-NS.
Another solution (pasted below) is to do conversions on the operands. This will prevent the Numeric Overflow Errors, but kind of decreases the readability of the code. Scaling up the operand values might not make them more accurate, but we should not _lose_ accuracy this way.
```java
if (TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime) > 2L * timeout) {
throw new RuntimeException("Failed to connect to zookeeper (" + host
+ ") within 2x zookeeper timeout period " + timeout);
}
```
```java
if (tryAgain) {
if (TimeUnit.NANOSECONDS.toMillis(startTime + 2L * TimeUnit.MILLISECONDS.toNanos(timeout))
< TimeUnit.NANOSECONDS
.toMillis(System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(sleepTime)
+ TimeUnit.MILLISECONDS.toNanos(connectTimeWait))) {
```
```java
sleepTime =
TimeUnit.NANOSECONDS.toMillis(startTime + 2L * TimeUnit.MILLISECONDS.toNanos(timeout)
- System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(connectTimeWait));
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1489:
Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1489: Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
URL: https://github.com/apache/accumulo/pull/1489#discussion_r373493480
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java
##########
@@ -119,8 +119,19 @@ public static ZooKeeper connect(String host, int timeout, String scheme, byte[]
boolean tryAgain = true;
long sleepTime = 100;
ZooKeeper zooKeeper = null;
-
- long startTime = System.currentTimeMillis();
+ /*
+ * Originally, startTime = System.currentTimeMillis(). Changed to System.nanoTime() to more
+ * accurately compute durations because it is not based on system clock variations. The
+ * ZooKeeper method signature expects an int value for 'timeout' and performs several
+ * calculations that can result in Numeric Expression Overflow errors if large, nanosecond
+ * values are used. For ths reason, System.nanoTime() is is converted to MS units prior to being
+ * used in calculations. Although, the resolution of 'startTime' is still in the MS range, the
+ * value from which it is calculated is nanoTime. Also, the MS units are preserved in the
+ * original method code, without the need for conversion. TimeUnit.convert is not used because
+ * "conversions from fine to coarser granularities truncate, so lose precision" (Oracle Java 7
+ * API)
+ */
+ long startTime = System.nanoTime() / 1000000;
Review comment:
@cradal I updated your comment's Markdown to use syntax highlighting, to make it easier to read, but I think it'd be easier if you just updated the PR with the new changes you are proposing, rather than paste it in to a comment. It will be easier to review the proposed changes inline, rather than in a discussion thread. Also, if you want to make your code more readable, you could statically import TimeUnit.NANOSECONDS and TimeUnit.MILLISECONDS.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] cradal commented on issue #1489: Fixes #1458 -
ZooSession now uses nanoTime vice currentTimeMillis
Posted by GitBox <gi...@apache.org>.
cradal commented on issue #1489: Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
URL: https://github.com/apache/accumulo/pull/1489#issuecomment-581417459
I incorporated your suggestion to calculate a duration value and then convert to milliseconds. Just to be overly thorough, I compared the previous equations with the updated ones to make sure they are mathematically equivalent. This compiles fine. However, on about half of the Sunnyday test runs, the 2x zimeout period RuntimeException is thrown. Since this is supposed to happen if duration > 2X timeout, should we be concerned if that is the only test failure?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1489:
Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1489: Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
URL: https://github.com/apache/accumulo/pull/1489#discussion_r373026444
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java
##########
@@ -119,8 +119,19 @@ public static ZooKeeper connect(String host, int timeout, String scheme, byte[]
boolean tryAgain = true;
long sleepTime = 100;
ZooKeeper zooKeeper = null;
-
- long startTime = System.currentTimeMillis();
+ /*
+ * Originally, startTime = System.currentTimeMillis(). Changed to System.nanoTime() to more
+ * accurately compute durations because it is not based on system clock variations. The
+ * ZooKeeper method signature expects an int value for 'timeout' and performs several
+ * calculations that can result in Numeric Expression Overflow errors if large, nanosecond
+ * values are used. For ths reason, System.nanoTime() is is converted to MS units prior to being
+ * used in calculations. Although, the resolution of 'startTime' is still in the MS range, the
+ * value from which it is calculated is nanoTime. Also, the MS units are preserved in the
+ * original method code, without the need for conversion. TimeUnit.convert is not used because
+ * "conversions from fine to coarser granularities truncate, so lose precision" (Oracle Java 7
+ * API)
+ */
+ long startTime = System.nanoTime() / 1000000;
Review comment:
It's not a good idea to divide here, since it is the differences between two nano-times that matter, and you are losing precision here in a way that could affect the subtraction later.
The correct pattern to do the conversion to millis would be something like this:
```java
long startTime = System.nanoTime();
// code here...
long laterTime = System.nanoTime();
long timeDurationMillis = TimeUnit.NANOSECONDS.toMillis(laterTime - startTime);
```
It is okay to use the TimeUnit conversions here, and lose precision, because it's okay to lose the precision in the difference. It is not okay to lose precision on the values before computing the difference in times. (Also, using this method to convert is the same as dividing... they both lose precision in the same way.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] ctubbsii commented on issue #1489: Fixes #1458 -
ZooSession now uses nanoTime vice currentTimeMillis
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1489: Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
URL: https://github.com/apache/accumulo/pull/1489#issuecomment-580757962
@cradal What about something like:
```diff
diff --git a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java
index 5ba096e1d8..b00ad817b5 100644
--- a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java
+++ b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java
@@ -19,6 +19,7 @@
package org.apache.accumulo.fate.zookeeper;
import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.concurrent.TimeUnit.NANOSECONDS;
import java.io.IOException;
import java.net.UnknownHostException;
@@ -120,7 +121,7 @@ public class ZooSession {
long sleepTime = 100;
ZooKeeper zooKeeper = null;
- long startTime = System.currentTimeMillis();
+ long startTime = System.nanoTime();
while (tryAgain) {
try {
@@ -155,14 +156,16 @@ public class ZooSession {
}
}
- if (System.currentTimeMillis() - startTime > 2L * timeout) {
+ long stopTime = System.nanoTime();
+ long duration = NANOSECONDS.toMillis(stopTime - startTime);
+ if (duration > 2L * timeout) {
throw new RuntimeException("Failed to connect to zookeeper (" + host
+ ") within 2x zookeeper timeout period " + timeout);
}
if (tryAgain) {
- if (startTime + 2L * timeout < System.currentTimeMillis() + sleepTime + connectTimeWait)
- sleepTime = startTime + 2L * timeout - System.currentTimeMillis() - connectTimeWait;
+ if (2L * timeout < duration + sleepTime + connectTimeWait)
+ sleepTime = 2L * timeout - duration - connectTimeWait;
if (sleepTime < 0) {
connectTimeWait -= sleepTime;
sleepTime = 0;
```
Note: I haven't tested this... not sure it compiles, and it might not cover everything.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] cradal commented on a change in pull request #1489:
Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1489: Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
URL: https://github.com/apache/accumulo/pull/1489#discussion_r373485314
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java
##########
@@ -119,8 +119,19 @@ public static ZooKeeper connect(String host, int timeout, String scheme, byte[]
boolean tryAgain = true;
long sleepTime = 100;
ZooKeeper zooKeeper = null;
-
- long startTime = System.currentTimeMillis();
+ /*
+ * Originally, startTime = System.currentTimeMillis(). Changed to System.nanoTime() to more
+ * accurately compute durations because it is not based on system clock variations. The
+ * ZooKeeper method signature expects an int value for 'timeout' and performs several
+ * calculations that can result in Numeric Expression Overflow errors if large, nanosecond
+ * values are used. For ths reason, System.nanoTime() is is converted to MS units prior to being
+ * used in calculations. Although, the resolution of 'startTime' is still in the MS range, the
+ * value from which it is calculated is nanoTime. Also, the MS units are preserved in the
+ * original method code, without the need for conversion. TimeUnit.convert is not used because
+ * "conversions from fine to coarser granularities truncate, so lose precision" (Oracle Java 7
+ * API)
+ */
+ long startTime = System.nanoTime() / 1000000;
Review comment:
Indeed, that makes more sense. Working the issue along those lines, there are two sections in ZooSession where calculations are performed using millis values for all the operands:
`if (System.currentTimeMillis() - startTime > 2L * timeout) {
throw new RuntimeException("Failed to connect to zookeeper (" + host
+ ") within 2x zookeeper timeout period " + timeout);
}`
`if (tryAgain) {
if (startTime + 2L * timeout < System.currentTimeMillis() + sleepTime + connectTimeWait)
sleepTime = startTime + 2L * timeout - System.currentTimeMillis() - connectTimeWait;
if (sleepTime < 0) {
connectTimeWait -= sleepTime;
sleepTime = 0;
}
UtilWaitThread.sleep(sleepTime);
if (sleepTime < 10000)
sleepTime = sleepTime + (long) (sleepTime * secureRandom.nextDouble());
}`
One solution is to convert from millis to nano when the values are first assigned. There are a couple issues I see with doing that. One, timeout is still being passed into the method in millis and would need to be converted at the point-of-use (or assigned a value within the method, which breaks convention). Two, there are calculations that use these values, but do not use startTime, that are already working correctly. Also, TIME_BETWEEN_CONNECT_CHECKS_MS would likely need to be refactored to ...-NS.
Another solution (pasted below) is to do conversions on the operands. This will prevent the Numeric Overflow Errors, but kind of decreases the readability of the code. Scaling up the operand values might not make them more accurate, but we should not _lose_ accuracy this way.
`if (TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime) > 2L * timeout) {
throw new RuntimeException("Failed to connect to zookeeper (" + host
+ ") within 2x zookeeper timeout period " + timeout);
}`
`if (tryAgain) {
if (TimeUnit.NANOSECONDS.toMillis(startTime + 2L * TimeUnit.MILLISECONDS.toNanos(timeout))
< TimeUnit.NANOSECONDS
.toMillis(System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(sleepTime)
+ TimeUnit.MILLISECONDS.toNanos(connectTimeWait))) {`
`sleepTime =
TimeUnit.NANOSECONDS.toMillis(startTime + 2L * TimeUnit.MILLISECONDS.toNanos(timeout)
- System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(connectTimeWait));
}`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [accumulo] cradal commented on a change in pull request #1489:
Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
Posted by GitBox <gi...@apache.org>.
cradal commented on a change in pull request #1489: Fixes #1458 - ZooSession now uses nanoTime vice currentTimeMillis
URL: https://github.com/apache/accumulo/pull/1489#discussion_r373485314
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooSession.java
##########
@@ -119,8 +119,19 @@ public static ZooKeeper connect(String host, int timeout, String scheme, byte[]
boolean tryAgain = true;
long sleepTime = 100;
ZooKeeper zooKeeper = null;
-
- long startTime = System.currentTimeMillis();
+ /*
+ * Originally, startTime = System.currentTimeMillis(). Changed to System.nanoTime() to more
+ * accurately compute durations because it is not based on system clock variations. The
+ * ZooKeeper method signature expects an int value for 'timeout' and performs several
+ * calculations that can result in Numeric Expression Overflow errors if large, nanosecond
+ * values are used. For ths reason, System.nanoTime() is is converted to MS units prior to being
+ * used in calculations. Although, the resolution of 'startTime' is still in the MS range, the
+ * value from which it is calculated is nanoTime. Also, the MS units are preserved in the
+ * original method code, without the need for conversion. TimeUnit.convert is not used because
+ * "conversions from fine to coarser granularities truncate, so lose precision" (Oracle Java 7
+ * API)
+ */
+ long startTime = System.nanoTime() / 1000000;
Review comment:
Indeed, that makes more sense. Working the issue along those lines, there are two sections in ZooSession where calculations are performed using millis values for all the operands:
```java
if (System.currentTimeMillis() - startTime > 2L * timeout) {
throw new RuntimeException("Failed to connect to zookeeper (" + host
+ ") within 2x zookeeper timeout period " + timeout);
}
```
```java
if (tryAgain) {
if (startTime + 2L * timeout < System.currentTimeMillis() + sleepTime + connectTimeWait)
sleepTime = startTime + 2L * timeout - System.currentTimeMillis() - connectTimeWait;
if (sleepTime < 0) {
connectTimeWait -= sleepTime;
sleepTime = 0;
}
UtilWaitThread.sleep(sleepTime);
if (sleepTime < 10000)
sleepTime = sleepTime + (long) (sleepTime * secureRandom.nextDouble());
}
```
One solution is to convert from millis to nano when the values are first assigned. There are a couple issues I see with doing that. One, timeout is still being passed into the method in millis and would need to be converted at the point-of-use (or assigned a value within the method, which breaks convention). Two, there are calculations that use these values, but do not use startTime, that are already working correctly. Also, TIME_BETWEEN_CONNECT_CHECKS_MS would likely need to be refactored to ...-NS.
Another solution (pasted below) is to do conversions on the operands. This will prevent the Numeric Overflow Errors, but kind of decreases the readability of the code. Scaling up the operand values might not make them more accurate, but we should not _lose_ accuracy this way.
```java
if (TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTime) > 2L * timeout) {
throw new RuntimeException("Failed to connect to zookeeper (" + host
+ ") within 2x zookeeper timeout period " + timeout);
}
```
```java
if (tryAgain) {
if (TimeUnit.NANOSECONDS.toMillis(startTime + 2L * TimeUnit.MILLISECONDS.toNanos(timeout))
< TimeUnit.NANOSECONDS
.toMillis(System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(sleepTime)
+ TimeUnit.MILLISECONDS.toNanos(connectTimeWait))) {
sleepTime =
TimeUnit.NANOSECONDS.toMillis(startTime + 2L * TimeUnit.MILLISECONDS.toNanos(timeout)
- System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(connectTimeWait));
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services