----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5054/#review7652 -----------------------------------------------------------
Ship it! Thanks for the patch Brock. One comment noted below. If you agree, please address it and update the patch on the Jira for commit. flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java <https://reviews.apache.org/r/5054/#comment16863> 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. - Arvind 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 > >
