----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4826/#review7102 -----------------------------------------------------------
Thanks for the patch, Inder! This looks to be very useful. Below are a few suggestions. flume-ng-core/src/main/java/org/apache/flume/event/EventHeaderDecorator.java <https://reviews.apache.org/r/4826/#comment15714> This abstract class should probably be an interface (it has no implementation). flume-ng-core/src/main/java/org/apache/flume/event/EventHeaderDecoratorFactory.java <https://reviews.apache.org/r/4826/#comment15716> Please don't use static variables. Just create a new instance every time from this factory. flume-ng-core/src/main/java/org/apache/flume/event/SinkEventHeaderDecorator.java <https://reviews.apache.org/r/4826/#comment15717> Does this class belong in the core at all? In my view, this class isn't general enough to be checked into the Flume core. This should be kept as an application-specific plugin which is added to the runtime classpath and referenced in the config. That's sort of the point of adding this EventHeaderDecorator plugin interface, right? - Mike On 2012-04-20 10:33:14, Inder Singh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4826/ > ----------------------------------------------------------- > > (Updated 2012-04-20 10:33:14) > > > Review request for Flume, Arvind Prabhakar, Mike Percy, and Hari Shreedharan. > > > Summary > ------- > > Overview of Changes > > 1.EventHeaderDecorator -> generic class having processHeader() > 2.SinkEventHeaderDecorator is a EventHeaderDecorator which overrides > timestamp header at this hop. > 3.EventHeaderDecoratorFactory provides EventHeaderDecorator's based on > <sink>.headerDecorator config > 4.AbstractSink -> has a eventHeaderDecortaor > 5.HDFSEventSink -> checks for decorator through config and calls > processHeader() for each event which modifies/add headers based on event > decorator defined through config. > > Sample Configuration for testing > ---------------------------------- > agent1.sinks.log-sink1.hdfs.headerDecorator = sinkDecorator > > > This addresses bug FLUME-1097. > https://issues.apache.org/jira/browse/FLUME-1097 > > > Diffs > ----- > > > flume-ng-core/src/main/java/org/apache/flume/event/EventHeaderDecorator.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/event/EventHeaderDecoratorFactory.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/event/SinkEventHeaderDecorator.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/sink/AbstractSink.java 2334059 > > flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java > 1b61cad > > Diff: https://reviews.apache.org/r/4826/diff > > > Testing > ------- > > Testing Done > ------------- > 1. Unit tests > 2. manual tests done - agent1.sinks.log-sink1.hdfs.filePrefix = flume-data/%D > results in publishing data with timestamp at agent's hop running HDFSEventSink > > > Thanks, > > Inder > >
