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