Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Flume, mail # dev - Re: Review Request 12060: Revised design for Spillable Mem Channel


Copy link to this message
-
Re: Review Request 12060: Revised design for Spillable Mem Channel
Hari Shreedharan 2013-10-16, 00:06

-----------------------------------------------------------
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).