----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12060/#review25798 -----------------------------------------------------------
Hi Roshan, This is starting to look pretty good. I did a quick-ish review, and have some comments. Also I am not entirely convinced we need the ChannelFullException - why would ChannelException itself not suffice here? Also, I think you need to rebase on the newer file channel updates, since the patch seems to need hadoop-common which File Channel no longer needs. flume-ng-channels/flume-spillable-memory-channel/pom.xml <https://reviews.apache.org/r/12060/#comment50322> This should no longer be needed, with the File Channel hadoop dependency gone, right? flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java <https://reviews.apache.org/r/12060/#comment50323> This should not be public right? We don't want people using the interface directly, but only through configs no? flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java <https://reviews.apache.org/r/12060/#comment50324> What does this signify? flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java <https://reviews.apache.org/r/12060/#comment50336> Why is this public? Can this be made final? flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java <https://reviews.apache.org/r/12060/#comment50328> I really think we need a better way of deciding if the events are in primary or secondary. We should make it easy to understand. This works, but I'd like it to be clearer (use an Enum or boolean to specify?) Agreed that this is more memory-efficient, at least it should be documented - so we can maintain it properly. flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java <https://reviews.apache.org/r/12060/#comment50332> Looks like none of the methods in this class are thread-safe and all methods are called from a synchronized(queueLock) block. I think it is better to actually make these methods thread-safe,and minimize the synchronization done using queueLock. flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java <https://reviews.apache.org/r/12060/#comment50330> Nit: Braces missing. Usually even if it is one line, we use braces (or put the statement in the same line as the if). flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java <https://reviews.apache.org/r/12060/#comment50331> Nit:space between - and eventCount flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java <https://reviews.apache.org/r/12060/#comment50335> What does this mean? Can you make the error message better here? flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java <https://reviews.apache.org/r/12060/#comment50334> lets have a different config to disable overflow, than overloading the same param. - Hari Shreedharan On July 3, 2013, 11:56 p.m., Roshan Naik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12060/ > ----------------------------------------------------------- > > (Updated July 3, 2013, 11:56 p.m.) > > > Review request for Flume. > > > Bugs: FLUME-1227 > https://issues.apache.org/jira/browse/FLUME-1227 > > > Repository: flume-git > > > Description > ------- > > Revised design for Spillable Mem Channel. > We no longer have Spillable channel config pointing to another channel (by > name) as in the previous design. > > Spillable Channel instead derives from FileChannel (as per > https://issues.apache.org/jira/browse/FLUME-1227?focusedCommentId=13628201&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13628201) > > Essence of this design: > - SC derives from File channel and maintains an in memory queue. If memory > queue is full, events are sent to disk overflow (i.e. File channel). > - SC maintains a 'Drain-Order' queue (DOQ) for remembering the order in which > the incoming events were interleaved between main memory and > - Put transaction: All the elements in the putList are committed to mem queue > if it has space, else written to disk (ie file channel). Head of DOQ is > updated to indicate where the elements put. > - Take transaction : Tail of DOQ is consulted to determine whether the next > set of events are to be taken from the memory queue or from disk overflow. > DOQ's tail is updated after events are taken out. > > > SC Configuration: > - Accepts all the File Channel settings > - Introduces one additional setting: 'memoryCapacity' which indicates the > number of items it can hold in memory > > > Sample config ... > > a1.channels = c1 > a1.sinks = logger > a1.sources = src > > a1.sources.src.type = exec > a1.sources.src.command = seq 1 100000 > a1.sources.src.batchSize = 10 > a1.sources.src.channels = c1 > > a1.sinks.logger.type = logger > a1.sinks.logger.channel = c1 > > a1.channels.c1.type = spillablememory > a1.channels.c1.checkpointDir = /tmp/flume/checkpoint > a1.channels.c1.dataDirs = /tmp/flume/data > a1.channels.c1.memoryCapacity = 10 > a1.channels.c1.keep-alive = 2 > > > Diffs > ----- > > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java > 36f150b > flume-ng-channels/flume-spillable-memory-channel/pom.xml PRE-CREATION > > flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java > PRE-CREATION > > flume-ng-channels/flume-spillable-memory-channel/src/test/java/org/apache/flume/channel/TestSpillableMemoryChannel.java > PRE-CREATION > flume-ng-channels/pom.xml 5832ab4 > > flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/ChannelConfiguration.java > 26f4dd7 > > flume-ng-configuration/src/main/java/org/apache/flume/conf/channel/ChannelType.java > 15b8cc3 > flume-ng-core/src/main/java/org/apache/flume/ChannelFullException.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/channel/AbstractChannel.java > 1370e66 > flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java > 688323d > flume-ng-dist/pom.xml 83332a9 > flume-ng-doc/sphinx/FlumeUserGuide.rst 63cad21 > > flume-ng-embedded-agent/src/main/java/org/apache/flume/agent/embedded/EmbeddedAgentConfiguration.java > 6204bc5 > flume-ng-node/pom.xml f1b0c65 > pom.xml 15e6d9b > > Diff: https://reviews.apache.org/r/12060/diff/ > > > Testing > ------- > > Wrote a set of Unit tests. A few are failing and need to be finished up. > > > File Attachments > ---------------- > > Revised design doc > > https://reviews.apache.org/media/uploaded/files/2013/06/24/SpillableMemory_Channel_Design_2_1.pdf > > > Thanks, > > Roshan Naik > >
