> On 2012-05-07 19:23:38, Arvind Prabhakar wrote: > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java, > > lines 393-398 > > <https://reviews.apache.org/r/5054/diff/2/?file=107578#file107578line393> > > > > potential thread safety issue: the loop is over the keySet view of the > > idLogFileMap and may not be synchronized. Suggest enclosing it in > > synchronized(idLogFileMap) block since it is also accessed by the > > background thread. > > > >
All code paths which use that check the open flag. However, I agree that it's better to be explicit here, the patch I added to the JIRA has this synchronized block. - Brock ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5054/#review7652 ----------------------------------------------------------- On 2012-05-07 18:39:04, Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5054/ > ----------------------------------------------------------- > > (Updated 2012-05-07 18:39:04) > > > Review request for Flume and Arvind Prabhakar. > > > Summary > ------- > > TestFileChannel.testThreaded has a race condition due to > FileChannel.FileBackedTransaction not blocking. Sometimes the take threads > will find no events on the queue and quit. This patch addresses this issue > and additionally addresses a few issues found in the Log class: > > 1) We are not closing files open for gets() > 2) removeOldLogs could be called after the log as been closed by the > background thread (identified while fixing #1). > > > This addresses bug FLUME-1184. > https://issues.apache.org/jira/browse/FLUME-1184 > > > Diffs > ----- > > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java > a777cd6 > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestFileChannel.java > d20e68c > > Diff: https://reviews.apache.org/r/5054/diff > > > Testing > ------- > > All unit tests pass and the unit test in question passed 1000 times in a row > which it had previously failed to do. > > > Thanks, > > Brock > >
