keith-turner commented on code in PR #6025:
URL: https://github.com/apache/accumulo/pull/6025#discussion_r2737290390
##########
server/base/src/main/java/org/apache/accumulo/server/tablets/TabletTime.java:
##########
@@ -87,6 +89,8 @@ public MetadataTime getMetadataTime(long time) {
}
@Override
+ @SuppressFBWarnings(value = "AT_NONATOMIC_64BIT_PRIMITIVE",
+ justification = "this is only called in tablet constructor, so does
not need to be done atomically")
public void useMaxTimeFromWALog(long time) {
Review Comment:
Would making this method syncronized remove the warning? Seems safe to add
the sync and it makes the code more tolerant of changes in how its used.
```suggestion
public synchronized void useMaxTimeFromWALog(long time) {
```
Also seems like the following methods in this class should be made
synchronized because they read and/or write the classes variables.
- `MetadataTime getMetadataTime()`
- `public long getTime()`
##########
server/base/src/main/java/org/apache/accumulo/server/compaction/CountingIterator.java:
##########
@@ -23,12 +23,15 @@
import java.util.Map;
import java.util.concurrent.atomic.AtomicLong;
+import javax.annotation.concurrent.NotThreadSafe;
+
import org.apache.accumulo.core.data.Key;
import org.apache.accumulo.core.data.Value;
import org.apache.accumulo.core.iterators.IteratorEnvironment;
import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
import org.apache.accumulo.core.iterators.WrappingIterator;
+@NotThreadSafe
public class CountingIterator extends WrappingIterator {
Review Comment:
This code is trying to accommodate a multi threaded use case of informing
other threads about the number of entries read via a AtomicLong. So not sure
if this annotation should be applied here.
##########
server/base/src/main/java/org/apache/accumulo/server/fs/FileManager.java:
##########
@@ -386,6 +388,7 @@ private void releaseReaders(KeyExtent tablet,
List<FileSKVIterator> readers,
}
+ @NotThreadSafe
static class FileDataSource implements DataSource {
Review Comment:
This is not ThreadSafe, however it may be used by multiple threads but all
usages of it seem to be inside synchronized FileManager method so it would need
its own sync.
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/InMemoryMap.java:
##########
@@ -531,6 +533,7 @@ public synchronized long getNumEntries() {
private final Set<MemoryIterator> activeIters =
Collections.synchronizedSet(new HashSet<>());
+ @NotThreadSafe
class MemoryDataSource implements DataSource {
Review Comment:
This method has a synchronized method and it trying handle the case of an
iterator being concurrently switched from in memory data to a file. Looking at
the bigger picture there is a lot of locking going on around this with syncs on
differnt objects. I would like to open a follow on issue to analyze this
further and see if simplifications can be made. What is the best thing to do
in the PR if we want a follow on?
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Scanner.java:
##########
@@ -213,6 +215,8 @@ public Thread getLockOwner() {
* read is in progress), it interrupts the reading thread. This ensures the
reading thread can
* finish its current operation and release the lock, allowing close to
finish.
*/
+ @SuppressFBWarnings(value = "AT_STALE_THREAD_WRITE_OF_PRIMITIVE",
+ justification = "accesses non-volatile/non-atomic vars only when lock is
held")
Review Comment:
Wonder if we could avoid this warning by adding a method like :
```java
private void lockAndClose(){
lock.lock();
try{
scanClosed = true;
if (isolatedDataSource != null) {
isolatedDataSource.close(false);
}
}finally {
lock.unlock();
}
}
```
and then calling that method in in `close()` only after tryLock was
successful. Locks are re entrant so it should ok to lock twice and maybe the
static analysis would know its only accessed when the lock is held.
This may be slightly better for maintainability as it makes it clear that
the lock must be help when accessing these fields in the object.
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/DistributedReadWriteLock.java:
##########
@@ -107,6 +109,7 @@ public interface QueueLock {
private static final Logger log =
LoggerFactory.getLogger(DistributedReadWriteLock.class);
+ @NotThreadSafe
Review Comment:
Could add a comment to the code w/ the annotation. Something like :
`Intended usage of instances of this class is that its only used by a single
thread and supports locking across threads/processes`.
--
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]
