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
+
Hari Shreedharan 2013-08-30, 20:41
Copy link to this message
-
Re: Review Request 12060: Revised design for Spillable Mem Channel


> On Aug. 30, 2013, 8:41 p.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-spillable-memory-channel/pom.xml, lines 65-70
> > <https://reviews.apache.org/r/12060/diff/3/?file=318118#file318118line65>
> >
> >     This should no longer be needed, with the File Channel hadoop dependency gone, right?

yes.
> On Aug. 30, 2013, 8:41 p.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java, line 72
> > <https://reviews.apache.org/r/12060/diff/3/?file=318119#file318119line72>
> >
> >     What does this signify?

should be defaultMemoryCapacity actually.
> On Aug. 30, 2013, 8:41 p.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java, lines 137-138
> > <https://reviews.apache.org/r/12060/diff/3/?file=318119#file318119line137>
> >
> >     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.

I assume you mean replace counts with bool/enum. Indeed using a stack of enums/bools means (one for every event) becomes very inefficient.

I assume you are suggesting that I add javadocs for the DrainOrderQueue class for better maintainability ? i would be happy to. please confirm.
> On Aug. 30, 2013, 8:41 p.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java, line 138
> > <https://reviews.apache.org/r/12060/diff/3/?file=318119#file318119line138>
> >
> >     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.

The drain order queue needs to be updated in a single atomic transaction along with an update to the primary or overflow. So the external coarser granularity lock is unavoidable.  Also there is a need to combine a few method invocations on DrainOrderQueue into single atomic transaction. (read/modify/write type operation)
So an internally synchronized DrainOrderQueue would introduce superfluous locking very frequently (multiple times per event).
> On Aug. 30, 2013, 8:41 p.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java, lines 158-162
> > <https://reviews.apache.org/r/12060/diff/3/?file=318119#file318119line158>
> >
> >     Nit: Braces missing. Usually even if it is one line, we use braces (or put the statement in the same line as the if).

ok.. i have updated the code to fix all such oneliner if/else  instances that i could find.
> On Aug. 30, 2013, 8:41 p.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java, line 421
> > <https://reviews.apache.org/r/12060/diff/3/?file=318119#file318119line421>
> >
> >     What does this mean? Can you make the error message better here?

Is this clearer?    "Unable to insert event into memory queue in spite of spare capacity, this is very unexpected"
> On Aug. 30, 2013, 8:41 p.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java, lines 600-604
> > <https://reviews.apache.org/r/12060/diff/3/?file=318119#file318119line600>
> >
> >     lets have a different config to disable overflow, than overloading the same param.

Ok. We will then need one config for disabling primary & one for disabling overflow.

Setting memoryCapacity or overflowCapacity to zero would still mean 'do not store anything' in them and consequently retain the same effect. So i didnt introduce additional configs. Let me know what you think.
- Roshan
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12060/#review25798
On July 3, 2013, 11:56 p.m., Roshan Naik wrote:
+
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