----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18544/#review56592 -----------------------------------------------------------
Hi Roshan, It looks good and I think we should commit this soon. There are just a couple things we should resolve. There is a bunch of code which is commented out. We should remove that. Also there is a bunch of lint/whitepsace issues which we should resolve. Can you go throught the patch and find any if(x==y) and make it if (x == y) flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java <https://reviews.apache.org/r/18544/#comment96959> are these changes due to the rev in the thrift version? flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveDelimitedTextSerializer.java <https://reviews.apache.org/r/18544/#comment96958> extra line flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveDelimitedTextSerializer.java <https://reviews.apache.org/r/18544/#comment96957> extra space flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveDelimitedTextSerializer.java <https://reviews.apache.org/r/18544/#comment96956> spaces flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveSink.java <https://reviews.apache.org/r/18544/#comment96955> map on LHS flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveSink.java <https://reviews.apache.org/r/18544/#comment96952> why is this here and then the same thing a few lines below not commented flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveSink.java <https://reviews.apache.org/r/18544/#comment96953> why is heartBeatTimer not private flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveSink.java <https://reviews.apache.org/r/18544/#comment96954> does this really need hashmap on LHS? Sould be map flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveSink.java <https://reviews.apache.org/r/18544/#comment96951> this context.getString(Config.SERIALIZER); should be context.getString(Config.SERIALIZER, "").trim(); and then you can check for s.isEmpty() in the next line and not worry about nulls. flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveSink.java <https://reviews.apache.org/r/18544/#comment96950> Why do we have HashMap on LHS, it should be Map flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveWriter.java <https://reviews.apache.org/r/18544/#comment96949> All of these classes below should end with Exception flume-ng-sinks/flume-hive-sink/src/test/java/org/apache/flume/sink/hive/TestHiveWriter.java <https://reviews.apache.org/r/18544/#comment96948> remove commented code flume-ng-sinks/flume-hive-sink/src/test/java/org/apache/flume/sink/hive/TestUtil.java <https://reviews.apache.org/r/18544/#comment96946> We should log exceptions like this. flume-ng-sinks/flume-hive-sink/src/test/java/org/apache/flume/sink/hive/TestUtil.java <https://reviews.apache.org/r/18544/#comment96947> missing spaces if(partKeys.size()!=partVals.size()) should be: if (partKeys.size() != partVals.size()) flume-ng-sinks/flume-hive-sink/src/test/java/org/apache/flume/sink/hive/TestUtil.java <https://reviews.apache.org/r/18544/#comment96945> We should remove commented code - Brock Noland On Sept. 3, 2014, 3:19 a.m., Roshan Naik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18544/ > ----------------------------------------------------------- > > (Updated Sept. 3, 2014, 3:19 a.m.) > > > Review request for Flume. > > > Bugs: FLUME-1734 > https://issues.apache.org/jira/browse/FLUME-1734 > > > Repository: flume-git > > > Description > ------- > > Hive streaming sink. > > > Diffs > ----- > > bin/flume-ng e09e26b > conf/log4j.properties 3918511 > > flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkConfiguration.java > ac11558 > > flume-ng-configuration/src/main/java/org/apache/flume/conf/sink/SinkType.java > 0a1cd7a > flume-ng-dist/pom.xml 8c18af6 > flume-ng-doc/sphinx/FlumeUserGuide.rst a718fbf > > flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java > ff32c45 > > flume-ng-legacy-sources/flume-thrift-source/src/test/java/org/apache/flume/source/thriftLegacy/TestThriftLegacySource.java > 8e08f22 > > flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java > 7f966b0 > flume-ng-sinks/flume-hive-sink/pom.xml PRE-CREATION > > flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/Config.java > PRE-CREATION > > flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveDelimitedTextSerializer.java > PRE-CREATION > > flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveEventSerializer.java > PRE-CREATION > > flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveJsonSerializer.java > PRE-CREATION > > flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveSink.java > PRE-CREATION > > flume-ng-sinks/flume-hive-sink/src/main/java/org/apache/flume/sink/hive/HiveWriter.java > PRE-CREATION > > flume-ng-sinks/flume-hive-sink/src/test/java/org/apache/flume/sink/hive/TestHiveSink.java > PRE-CREATION > > flume-ng-sinks/flume-hive-sink/src/test/java/org/apache/flume/sink/hive/TestHiveWriter.java > PRE-CREATION > > flume-ng-sinks/flume-hive-sink/src/test/java/org/apache/flume/sink/hive/TestUtil.java > PRE-CREATION > flume-ng-sinks/flume-hive-sink/src/test/resources/log4j.properties > PRE-CREATION > flume-ng-sinks/pom.xml 3381bde > > flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java > eba8d2e > pom.xml 4bdfcac > > Diff: https://reviews.apache.org/r/18544/diff/ > > > Testing > ------- > > includes unit tests. > > > Thanks, > > Roshan Naik > >
