You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Roshan Naik <ro...@hortonworks.com> on 2014/09/03 05:19:04 UTC
Re: Review Request 18544: Hive Streaming sink
-----------------------------------------------------------
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.
Changes
-------
adressing brock's review comments
Bugs: FLUME-1734
https://issues.apache.org/jira/browse/FLUME-1734
Repository: flume-git
Description
-------
Hive streaming sink.
Diffs (updated)
-----
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 (updated)
-------
includes unit tests.
Thanks,
Roshan Naik
Re: Review Request 18544: Hive Streaming sink
Posted by Roshan Naik <ro...@hortonworks.com>.
> On Oct. 14, 2014, 11:05 p.m., Brock Noland wrote:
> > flume-ng-sinks/flume-hive-sink/src/test/java/org/apache/flume/sink/hive/TestUtil.java, line 136
> > <https://reviews.apache.org/r/18544/diff/5/?file=674780#file674780line136>
> >
> > We should log exceptions like this.
loogging not yet been added in for unit tests. this class is used for unit tests.
> On Oct. 14, 2014, 11:05 p.m., Brock Noland wrote:
> > flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java, line 212
> > <https://reviews.apache.org/r/18544/diff/5/?file=674768#file674768line212>
> >
> > are these changes due to the rev in the thrift version?
yes. my notes suggest a race condition in TThreadSeverPool
- Roshan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18544/#review56592
-----------------------------------------------------------
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
>
>
Re: Review Request 18544: Hive Streaming sink
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 18544: Hive Streaming sink
Posted by Roshan Naik <ro...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18544/
-----------------------------------------------------------
(Updated Nov. 13, 2014, 3:05 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 (updated)
-----
bin/flume-ng 4b323a6
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 a5db0c7
flume-ng-doc/sphinx/FlumeUserGuide.rst 0ab23fd
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 4bac019
flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java eba8d2e
pom.xml 4f550d3
Diff: https://reviews.apache.org/r/18544/diff/
Testing
-------
includes unit tests.
Thanks,
Roshan Naik