You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "cshannon (via GitHub)" <gi...@apache.org> on 2024/09/07 23:08:28 UTC
[PR] Update ThritScanner to use nanotime [accumulo]
cshannon opened a new pull request, #4866:
URL: https://github.com/apache/accumulo/pull/4866
Update `ThriftScanner` to use nano time to be consistent with elapsed time tracking in other locations. Also, convert `timeOut` to be a Duration to prevent future errors. This value was being converted to seconds from millis which is error prone as there was not a good indication the unit was in seconds.
This is a follow on to #4864
As a side note, there are plenty of other timeout related properties that are part of the scanner code. I started to convert those as well to Duration but it cascaded and started touching a lot of classes which i didn't think was a good idea for 2.1. However, I was thinking as part of 3.1 and 4.0 we may want to update some more fields in classes from a long to a Duration (assuming no performance concerns). Here are some examples I found but I am sure there are more spots:
[TimeoutTabletLocator](https://github.com/apache/accumulo/blob/2a9aa76e97ac7b0359a7748f753313a992ebbb46/core/src/main/java/org/apache/accumulo/core/clientImpl/TimeoutTabletLocator.java#L42) (in 3.1 only)
[ScannerOptions](https://github.com/apache/accumulo/blob/2a9aa76e97ac7b0359a7748f753313a992ebbb46/core/src/main/java/org/apache/accumulo/core/clientImpl/ScannerOptions.java#L56-L58)
[TabletServerBatchReaderIterator](https://github.com/apache/accumulo/blob/c2e4874986157af720473af46d77030716e3009b/core/src/main/java/org/apache/accumulo/core/clientImpl/TabletServerBatchReaderIterator.java#L117)
[ScanState](https://github.com/apache/accumulo/blob/c2e4874986157af720473af46d77030716e3009b/core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftScanner.java#L225) in `ThriftScanner`
--
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] Update ThritScanner to use nanotime [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #4866:
URL: https://github.com/apache/accumulo/pull/4866#discussion_r1750928382
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ScannerImpl.java:
##########
@@ -163,7 +163,7 @@ public synchronized int getBatchSize() {
public synchronized Iterator<Entry<Key,Value>> iterator() {
ensureOpen();
ScannerIterator iter = new ScannerIterator(context, tableId, authorizations, range, size,
- getTimeout(SECONDS), this, isolated, readaheadThreshold, new Reporter());
+ Duration.ofMillis(retryTimeout), this, isolated, readaheadThreshold, new Reporter());
Review Comment:
With the following change less code is making assumptions about the units of the `retryTimeout` instance variable.
```suggestion
Duration.ofMillis(getTimeout(MILLISECONDS)), this, isolated, readaheadThreshold, new Reporter());
```
I was wondering if `getTimeout()` could be converted to return a Duration. Looking at how its used that kinda cascaded, which is what I think you mentioned in the PR description.
--
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] Update ThritScanner to use nanotime [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Update ThriftScanner to use nanotime [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4866:
URL: https://github.com/apache/accumulo/pull/4866#discussion_r1756610085
##########
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:
Thanks for the suggestion, this definitely is the better way to do it, I'm making the changes now.
--
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] Update ThriftScanner to use nanotime [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #4866:
URL: https://github.com/apache/accumulo/pull/4866#discussion_r1756624526
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ScannerImpl.java:
##########
@@ -163,7 +163,7 @@ public synchronized int getBatchSize() {
public synchronized Iterator<Entry<Key,Value>> iterator() {
ensureOpen();
ScannerIterator iter = new ScannerIterator(context, tableId, authorizations, range, size,
- getTimeout(SECONDS), this, isolated, readaheadThreshold, new Reporter());
+ Duration.ofMillis(retryTimeout), this, isolated, readaheadThreshold, new Reporter());
Review Comment:
Good point on using getTimeout() method, I'll change that.
In regards to converting getTimeout() to a Duration...right that is the exact problem I mentioned in my PR, it cascades very quickly and starts to impact several other classes as the timeouts are passed around so changing it to a Duration means changing a lot of other stuff. Also, for example, there's also getBatchTimeout() and I don't think it makes sense to change one and not the other.
So that's why I was thinking it would be better to minimize the changes in 2.1 at this point. We could make it a follow on for 3.1 and main. However, if it you think it's worthwhile for 2.1 I can still make the changes in 2.1 (even if it's another PR)
--
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] Update ThriftScanner to use nanotime [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #4866:
URL: https://github.com/apache/accumulo/pull/4866#issuecomment-2346449657
The MiniAccumulo tests were hanging on this and I took a look at logs and found the following:
```
2024-09-12T10:20:28,962 [threads.AccumuloUncaughtExceptionHandler] ERROR: Caught an Exception in Thread[accumulo.pool
.metadata.tablet.assignment-Worker-10,5,main]. Thread is dead.
java.lang.RuntimeException: java.lang.ArithmeticException: long overflow
at org.apache.accumulo.tserver.AssignmentHandler.run(AssignmentHandler.java:142) ~[accumulo-tserver-2.1.4-SNAPSHOT.jar:2.1.4-SNAPSHOT]
at org.apache.accumulo.tserver.ActiveAssignmentRunnable.run(ActiveAssignmentRunnable.java:63) ~[accumulo-tserver-2.1.4-SNAPSHOT.jar:2.1.4-SNAPSHOT]
at org.apache.accumulo.core.trace.TraceWrappedRunnable.run(TraceWrappedRunnable.java:52) ~[accumulo-core-2.1.4-SNAPSHOT.jar:2.1.4-SNAPSHOT]
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?]
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?]
at org.apache.accumulo.core.trace.TraceWrappedRunnable.run(TraceWrappedRunnable.java:52) ~[accumulo-core-2.1.4-SNAPSHOT.jar:2.1.4-SNAPSHOT]
at java.base/java.lang.Thread.run(Thread.java:840) [?:?]
Caused by: java.lang.ArithmeticException: long overflow
at java.base/java.lang.Math.multiplyExact(Math.java:1004) ~[?:?]
at java.base/java.time.Duration.toNanos(Duration.java:1250) ~[?:?]
at org.apache.accumulo.core.util.Timer.hasElapsed(Timer.java:62) ~[accumulo-core-2.1.4-SNAPSHOT.jar:2.1.4-SNAPSHOT]
at org.apache.accumulo.core.clientImpl.ThriftScanner.scan(ThriftScanner.java:344) ~[accumulo-core-2.1.4-SNAPSHOT.jar:2.1.4-SNAPSHOT]
```
--
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] Update ThriftScanner to use nanotime [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon merged PR #4866:
URL: https://github.com/apache/accumulo/pull/4866
--
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