Github user cestella commented on a diff in the pull request:

    https://github.com/apache/incubator-metron/pull/381#discussion_r90464946
  
    --- Diff: 
metron-platform/metron-test-utilities/src/main/java/org/apache/metron/test/mock/MockHTable.java
 ---
    @@ -437,15 +463,14 @@ public ResultScanner getScanner(byte[] family, byte[] 
qualifier)
         return getScanner(scan);
       }
     
    -  List<Put> putLog = new ArrayList<>();
    -
       public List<Put> getPutLog() {
    -    return putLog;
    +    return ImmutableList.copyOf(putLog.get());
       }
     
       @Override
       public void put(Put put) throws IOException {
    -    putLog.add(put);
    +    putLog.get().add(put);
    --- End diff --
    
    To @justinleet's point, while the `Supplier.get()` call is thread-safe, the 
resulting list is not.  I would recommend adjusting the construction of the 
`ArrayList<Put>` to `Collections.synchronizedList(new ArrayList<>())` to ensure 
that the resulting `add` call is happening in a thread-safe way.  At that 
point, it's unclear why we would use the supplier at all.  
    
    On the whole, it's interesting that this may be at the root of the unit 
test weirdness.  Have you gotten the tests to fail in a somewhat repeatable 
manner and does this change correct it?
    
    Standard disclaimer applies: all of this is based on my understanding of 
Java, which can be obviously incomplete, so let me know if I got something 
wrong here.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to