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

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


+
Roshan Naik 2013-07-03, 23:56
Copy link to this message
-
Re: Review Request 12060: Revised design for Spillable Mem Channel
Hari Shreedharan 2013-08-30, 20:41

-----------------------------------------------------------
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.
+
Roshan Naik 2013-10-04, 21:49
+
Roshan Naik 2013-10-15, 20:44
+
Hari Shreedharan 2013-10-16, 00:06
+
Roshan Naik 2013-10-24, 22:16
+
Roshan Naik 2013-10-24, 22:16
+
Roshan Naik 2013-10-24, 22:18
+
Hari Shreedharan 2013-10-31, 01:48
+
Roshan Naik 2013-10-31, 23:32
+
Roshan Naik 2013-10-31, 23:59
+
Roshan Naik 2013-11-05, 03:34
+
Roshan Naik 2013-12-18, 11:43
+
Hari Shreedharan 2013-10-31, 19:32