> On 2012-04-19 09:12:40, Arvind Prabhakar wrote: > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FlumeEventQueue.java, > > line 214 > > <https://reviews.apache.org/r/4661/diff/6/?file=102330#file102330line214> > > > > From cursory analysis it seems that this will likely not be able to > > handle the wrap-around logic correctly. For example, if the capacity is 10, > > next is 9 and size/index is 1: the calculated index will be 10, when it > > should be 0. > > > > One way to address this would be modulo the converted value before > > return: > > > > return (next + index % elements.capacity()) % elements.capacity(); > >
I am working on the rest of the feedback, but I just wanted to say "big thank you" for finding this bug. I hit it last night while stress testing and was planning on spending much of my day finding it. - Brock ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4661/#review6786 ----------------------------------------------------------- On 2012-04-17 06:36:33, Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4661/ > ----------------------------------------------------------- > > (Updated 2012-04-17 06:36:33) > > > Review request for Flume. > > > Summary > ------- > > This patch implements a durable file channel. It does by writing all > transaction events to disk and syncing to disk when a commit occurs. It does > have a memory component in that pointers to the event on disk are kept in > memory. This will consume 8 bytes of direct memory (non-heap) per event. Some > basic calculations are in the FileChannel java docs. > > > This addresses bug FLUME-1085. > https://issues.apache.org/jira/browse/FLUME-1085 > > > Diffs > ----- > > flume-ng-channels/flume-file-channel/pom.xml e8155be > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Checkpoint.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Commit.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java > a279453 > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannelConfiguration.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FlumeEvent.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FlumeEventPointer.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FlumeEventQueue.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFile.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogUtils.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Pair.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Put.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/ReplayHandler.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Rollback.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Take.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/TransactionEventRecord.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/CountingSinkRunner.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/CountingSourceRunner.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestCheckpoint.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestFileChannel.java > ab66998 > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestFlumeEvent.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestFlumeEventPointer.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestFlumeEventQueue.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestLog.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestLogFile.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestTransactionEventRecord.java > PRE-CREATION > > flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestUtils.java > PRE-CREATION > flume-ng-channels/flume-file-channel/src/test/resources/log4j.properties > 739ecc8 > > flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/RecoverableMemoryChannel.java > 0622f27 > > flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/wal/SequenceIDBuffer.java > fa63b73 > flume-ng-core/src/main/java/org/apache/flume/channel/ChannelType.java > d8419e8 > flume-ng-core/src/main/java/org/apache/flume/sink/NullSink.java c812851 > flume-ng-core/src/main/java/org/apache/flume/source/StressSource.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/tools/DirectMemoryUtils.java > PRE-CREATION > flume-ng-sdk/src/test/java/org/apache/flume/api/TestFailoverRpcClient.java > 225cd34 > flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java > 0b8a2c0 > > Diff: https://reviews.apache.org/r/4661/diff > > > Testing > ------- > > Unit tests and integration tests added to cover obvious cases. > > > Thanks, > > Brock > >
