keith-turner commented on code in PR #6025:
URL: https://github.com/apache/accumulo/pull/6025#discussion_r2636604446
##########
core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java:
##########
@@ -177,6 +177,7 @@ public ClientSideIteratorScanner(final Scanner scanner) {
this.range = scanner.getRange();
this.size = scanner.getBatchSize();
this.retryTimeout = scanner.getTimeout(MILLISECONDS);
+ // TODO Is this intended to be getBatchTimeout()?
Review Comment:
That does seem wrong.
##########
server/base/src/main/java/org/apache/accumulo/server/tablets/TabletTime.java:
##########
@@ -157,11 +160,9 @@ private LogicalTime(Long time) {
@Override
public void useMaxTimeFromWALog(long time) {
- time++;
+ final long finalTime = time + 1;
- if (this.nextTime.get() < time) {
- this.nextTime.set(time);
- }
+ this.nextTime.getAndUpdate(val -> Math.max(val, finalTime));
Review Comment:
Maybe do not need finalTime.
```suggestion
this.nextTime.getAndUpdate(val -> Math.max(val, time + 1));
```
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Scanner.java:
##########
@@ -24,23 +24,22 @@
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.locks.ReentrantLock;
+import javax.annotation.concurrent.NotThreadSafe;
+
import org.apache.accumulo.core.data.Key;
import org.apache.accumulo.core.data.Range;
import org.apache.accumulo.core.data.Value;
import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
import
org.apache.accumulo.core.iteratorsImpl.system.IterationInterruptedException;
import org.apache.accumulo.core.iteratorsImpl.system.SourceSwitchingIterator;
-import org.apache.accumulo.core.trace.ScanInstrumentation;
-import org.apache.accumulo.core.trace.TraceUtil;
-import org.apache.accumulo.core.util.Pair;
import org.apache.accumulo.core.util.ShutdownUtil;
-import org.apache.accumulo.tserver.scan.NextBatchTask;
import org.apache.accumulo.tserver.scan.ScanParameters;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import com.google.common.base.Preconditions;
+@NotThreadSafe
public class Scanner {
Review Comment:
This particular class does have multi threading support. Its close() method
is intended to be called by another thread different than the one reading data.
What was being complained about in this class?
##########
server/base/src/main/java/org/apache/accumulo/server/tablets/TabletTime.java:
##########
@@ -67,6 +69,7 @@ public static MetadataTime maxMetadataTime(MetadataTime mv1,
MetadataTime mv2) {
return mv1.compareTo(mv2) < 0 ? mv2 : mv1;
}
+ @NotThreadSafe
static class MillisTime extends TabletTime {
Review Comment:
Seems like a subset of the methods in TabletTime could be called by multiple
threads. So probably do not want to make this as `@NotThreadSafe`
--
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]
