----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12060/#review27047 -----------------------------------------------------------
Hey Roshan, I did a review of the patch - generally looks good. I have some comments though. Apart from the comments below, there are a lot of style issues that need to be taken care of. Also, the drain order queue needs to be documented better. In addition, I think the SpillableTxn class has a lot of conditionals which probably can be simplified. There is a bigger problem, which is the whole put/take methods being enclosed in synchronized blocks - locking the entire channel up. That would be ok, if the operation done was just polling of the queue, but this seems a little heavyweight to block the entire channel off. Can that be simplified/improved? flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java <https://reviews.apache.org/r/12060/#comment52662> Why is this being made protected? This is a File Channel internal component - we could change the implementation of the channel and get rid of the Log class altogether, so we should not expose this. flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java <https://reviews.apache.org/r/12060/#comment52660> This is exposing a File Channel internal data structure to a new component. Spillable Channel should use super.getTransaction() method to get the new transaction flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java <https://reviews.apache.org/r/12060/#comment52661> This should be private/package-private(like it is now). We should not expose this to a new component. flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java <https://reviews.apache.org/r/12060/#comment52663> These initial numbers seem a bit low. Let's bump this to 100 flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java <https://reviews.apache.org/r/12060/#comment52665> This essentially blocks more than 1 transaction at a time right? That will be a serious performance issue. flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java <https://reviews.apache.org/r/12060/#comment52666> Why only when !overflow? flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java <https://reviews.apache.org/r/12060/#comment52667> As mentioned above, this should not depend on FileBackedTxn flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java <https://reviews.apache.org/r/12060/#comment52669> Evne though gc will come and clear up all this, it might just be a good idea to clear up just for clarity and to help gc - Hari Shreedharan On Oct. 15, 2013, 8:44 p.m., Roshan Naik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12060/ > ----------------------------------------------------------- > > (Updated Oct. 15, 2013, 8:44 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 2d0ee47 > flume-ng-doc/sphinx/FlumeUserGuide.rst 98859ce > > flume-ng-embedded-agent/src/main/java/org/apache/flume/agent/embedded/EmbeddedAgentConfiguration.java > 6204bc5 > flume-ng-node/pom.xml f1b0c65 > pom.xml 267925f > > 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 > >
