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


> On Oct. 16, 2013, 12:06 a.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java, lines 370-374
> > <https://reviews.apache.org/r/12060/diff/4/?file=364810#file364810line370>
> >
> >     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

Yes. Will use super.createTransaction() instead.
> On Oct. 16, 2013, 12:06 a.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java, line 302
> > <https://reviews.apache.org/r/12060/diff/4/?file=364812#file364812line302>
> >
> >     This essentially blocks more than 1 transaction at a time right? That will be a serious performance issue.

I can see your point. However, my performance measurements do not seem to indicate perf issues with multiple sources & sinks (and memory queue disabled) when compared to FC. If you have some ideas around specific scenarios that could expose this perf issue, I could probe it further. The micro benchmark code for SpillChannel is built into the test: TestSpillableChannel.testParallelMultipleSourcesAndSinks in the patch.
> On Oct. 16, 2013, 12:06 a.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java, lines 339-341
> > <https://reviews.apache.org/r/12060/diff/4/?file=364812#file364812line339>
> >
> >     Why only when !overflow?

I assume you are enquiring about the comment.
Since in this case its operating only on takeList which is thread private. So no race condition here.
> On Oct. 16, 2013, 12:06 a.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java, line 87
> > <https://reviews.apache.org/r/12060/diff/4/?file=364810#file364810line87>
> >
> >     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.

was not required anymore. fixed.
> On Oct. 16, 2013, 12:06 a.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java, line 433
> > <https://reviews.apache.org/r/12060/diff/4/?file=364810#file364810line433>
> >
> >     This should be private/package-private(like it is now). We should not expose this to a new component.

not required anymore. fixed.
> On Oct. 16, 2013, 12:06 a.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java, lines 233-234
> > <https://reviews.apache.org/r/12060/diff/4/?file=364812#file364812line233>
> >
> >     These initial numbers seem a bit low. Let's bump this to 100
> On Oct. 16, 2013, 12:06 a.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java, lines 475-479
> > <https://reviews.apache.org/r/12060/diff/4/?file=364812#file364812line475>
> >
> >     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
> On Oct. 16, 2013, 12:06 a.m., Hari Shreedharan wrote:
> > flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java, lines 348-350
> > <https://reviews.apache.org/r/12060/diff/4/?file=364812#file364812line348>
> >
> >     As mentioned above, this should not depend on FileBackedTxn

Will use super.createTransaction() instead.
- Roshan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12060/#review27047
--------------------------------------------------

On Oct. 24, 2013, 10:16 p.m., Roshan Naik wrote: