ASF GitHub Bot commented on METRON-601:

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

    --- Diff: 
    @@ -437,15 +463,14 @@ public ResultScanner getScanner(byte[] family, byte[] 
         return getScanner(scan);
    -  List<Put> putLog = new ArrayList<>();
       public List<Put> getPutLog() {
    -    return putLog;
    +    return ImmutableList.copyOf(putLog.get());
       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.

> MockHTable Put Log is Not Thread Safe
> -------------------------------------
>                 Key: METRON-601
>                 URL: https://issues.apache.org/jira/browse/METRON-601
>             Project: Metron
>          Issue Type: Sub-task
>            Reporter: Nick Allen
>            Assignee: Nick Allen
> The MockHTable uses an ArrayList to store a log of Puts that have been 
> submitted against the MockHTable.  The MockHTable, along with the put log, is 
> accessed from multiple threads during the integration tests.  Access to the 
> Put log is not thread safe, which is likely at least one cause of METRON-597.
> The Put log is used by multiple tests, but more so by the 
> ProfilerIntegrationTest.  This tests polls the list to block the thread until 
> the expected number of Puts have been submitted.  This is likely why this 
> test is more impacted by this issue than others.
> The Put Log needs to made thread safe.  See 
> `org.apache.metron.test.mock.MockHTable.putLog`

This message was sent by Atlassian JIRA

Reply via email to