----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5347/#review8814 -----------------------------------------------------------
Thanks for the contribution, Mubarak. I don't know much about Redis, and I haven't tested this myself, but I have the following feedback on the patch. trunk/flume-ng-sinks/flume-ng-redis-sink/pom.xml <https://reviews.apache.org/r/5347/#comment18645> Version should be moved into the dependencyManagement section of the top-level pom file. trunk/flume-ng-sinks/flume-ng-redis-sink/pom.xml <https://reviews.apache.org/r/5347/#comment18711> extra tab character here trunk/flume-ng-sinks/flume-ng-redis-sink/src/main/java/org/apache/flume/sink/RedisEventSerializer.java <https://reviews.apache.org/r/5347/#comment18712> The files in this patch seem to be in the wrong location for this package (org/apache/flume/sink instead of org/apache/flume/sink/redis) trunk/flume-ng-sinks/flume-ng-redis-sink/src/main/java/org/apache/flume/sink/RedisEventSerializer.java <https://reviews.apache.org/r/5347/#comment18647> Nit: trailing whitespaces - please remove all of these (they show up as red on the review board) trunk/flume-ng-sinks/flume-ng-redis-sink/src/main/java/org/apache/flume/sink/RedisSink.java <https://reviews.apache.org/r/5347/#comment18713> Personally, I think it's better to only return BACKOFF if there were no events in the batch. Otherwise, you would get higher latency when the event volume is low. trunk/flume-ng-sinks/flume-ng-redis-sink/src/main/java/org/apache/flume/sink/RedisSink.java <https://reviews.apache.org/r/5347/#comment18715> Typically, the close() goes within a finally block and you rightfully catch a Throwable. Let's change this to use the typical idiom. The idiom / best practice for process() in sinks is: txn.begin(); try { // take events from channel in a batch // process/deliver your batch of events txn.commit(); } catch (Throwable t) { txn.rollback(); // re-throw Error // re-throw EventDeliveryException // wrap everything else (including RuntimeExceptions) in an EventDeliveryException then throw it. Add a descriptive message to the thing as well. } finally { txn.close(); } trunk/flume-ng-sinks/flume-ng-redis-sink/src/main/java/org/apache/flume/sink/RedisSink.java <https://reviews.apache.org/r/5347/#comment18714> I think this is over-defensive. When will rollback ever fail? - Mike Percy On June 27, 2012, 1:30 a.m., Mubarak Seyed wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5347/ > ----------------------------------------------------------- > > (Updated June 27, 2012, 1:30 a.m.) > > > Review request for Flume. > > > Description > ------- > > Redis supports pub/sub out-of-the-box. Flume based event streaming > application can make use of Redis publisher sink to publish events to > topic(s) in Redis and then subscriber(s) of topic/channel can receive events > to do near real-time analytics. > > > This addresses bug FLUME-1251. > https://issues.apache.org/jira/browse/FLUME-1251 > > > Diffs > ----- > > > trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkConfiguration.java > 1345351 > > trunk/flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkType.java > 1345351 > trunk/flume-ng-dist/pom.xml 1345351 > trunk/flume-ng-sinks/flume-ng-redis-sink/pom.xml PRE-CREATION > > trunk/flume-ng-sinks/flume-ng-redis-sink/src/main/java/org/apache/flume/sink/RedisEventSerializer.java > PRE-CREATION > > trunk/flume-ng-sinks/flume-ng-redis-sink/src/main/java/org/apache/flume/sink/RedisSink.java > PRE-CREATION > > trunk/flume-ng-sinks/flume-ng-redis-sink/src/main/java/org/apache/flume/sink/SimpleRedisEventSerializer.java > PRE-CREATION > > trunk/flume-ng-sinks/flume-ng-redis-sink/src/test/java/org/apache/flume/sink/redis/TestRedisSink.java > PRE-CREATION > trunk/flume-ng-sinks/pom.xml 1345351 > trunk/pom.xml 1345351 > > Diff: https://reviews.apache.org/r/5347/diff/ > > > Testing > ------- > > Yes, tested in lab environment. > > > Thanks, > > Mubarak Seyed > >
