sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Boglarka Egyed <b...@apache.org>
Subject Re: Review Request 66475: SQOOP-3310 Add import to Kafka feature
Date Fri, 06 Apr 2018 15:48:17 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66475/#review200652
-----------------------------------------------------------



Hi Andras,

Thanks for taking care of this new addition!

As this is a very huge change please note that this is just an initial review from me, I will
continue next week.

Could you please regenerate your patch? I got the following warning during applying it:

/Users/boglarka.egyed/Downloads/kafka.diff:868: new blank line at EOF.
+
/Users/boglarka.egyed/Downloads/kafka.diff:914: new blank line at EOF.
+

Thanks,
Bogi


ivy.xml
Lines 141 (patched)
<https://reviews.apache.org/r/66475/#comment281459>

    Unnecessary blank line.



ivy.xml
Lines 150 (patched)
<https://reviews.apache.org/r/66475/#comment281460>

    Unnecessary blank line.



src/java/org/apache/sqoop/SqoopOptions.java
Lines 769 (patched)
<https://reviews.apache.org/r/66475/#comment281455>

    typo: Netsted



src/java/org/apache/sqoop/kafka/GenericRecordKafkaTransformer.java
Lines 38 (patched)
<https://reviews.apache.org/r/66475/#comment281458>

    Unnecessary blank line.



src/java/org/apache/sqoop/kafka/GenericRecordKafkaTransformer.java
Lines 52-53 (patched)
<https://reviews.apache.org/r/66475/#comment281456>

    Unnecessary blank lines.



src/java/org/apache/sqoop/kafka/GenericRecordKafkaTransformer.java
Lines 72-73 (patched)
<https://reviews.apache.org/r/66475/#comment281457>

    Unnecessary blank lines.



src/java/org/apache/sqoop/kafka/KafkaProduceProcessor.java
Lines 60 (patched)
<https://reviews.apache.org/r/66475/#comment281461>

    Unnecessary blank line.



src/java/org/apache/sqoop/kafka/KafkaProduceProcessor.java
Lines 72 (patched)
<https://reviews.apache.org/r/66475/#comment281462>

    Unnecessary blank line.



src/java/org/apache/sqoop/kafka/KafkaProduceProcessor.java
Lines 120-122 (patched)
<https://reviews.apache.org/r/66475/#comment281463>

    Unnecessary blank lines.



src/java/org/apache/sqoop/kafka/KafkaProduceProcessor.java
Lines 145 (patched)
<https://reviews.apache.org/r/66475/#comment281464>

    Unnecessary blank line.



src/java/org/apache/sqoop/kafka/KafkaUtil.java
Lines 37 (patched)
<https://reviews.apache.org/r/66475/#comment281466>

    typo: testing and simulation the condition



src/java/org/apache/sqoop/kafka/KafkaUtil.java
Lines 38 (patched)
<https://reviews.apache.org/r/66475/#comment281465>

    typo: cliet



src/java/org/apache/sqoop/kafka/KafkaUtil.java
Lines 47-52 (patched)
<https://reviews.apache.org/r/66475/#comment281467>

    I know that Sqoop implementation already uses this logic at some places but I think it
would be much more clean to not have test related logic in the production code. Would you
mind to modify your change to eliminate this part and use mocking in tests where needed?



src/java/org/apache/sqoop/kafka/KafkaUtil.java
Lines 94-95 (patched)
<https://reviews.apache.org/r/66475/#comment281468>

    Unnecessary blank lines.



src/java/org/apache/sqoop/kafka/ProducerRecordTransformer.java
Lines 50 (patched)
<https://reviews.apache.org/r/66475/#comment281469>

    I see two input parameters not just one, could you please include that one here too?



src/java/org/apache/sqoop/kafka/ProducerWrapper.java
Lines 78 (patched)
<https://reviews.apache.org/r/66475/#comment281473>

    typo: send (sent?)



src/java/org/apache/sqoop/tool/BaseSqoopTool.java
Lines 937 (patched)
<https://reviews.apache.org/r/66475/#comment281481>

    typo: two consecutive whitespace


- Boglarka Egyed


On April 5, 2018, 12:35 p.m., Andras Beni wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66475/
> -----------------------------------------------------------
> 
> (Updated April 5, 2018, 12:35 p.m.)
> 
> 
> Review request for Sqoop, Boglarka Egyed, Fero Szabo, and Szabolcs Vasas.
> 
> 
> Bugs: SQOOP-3310
>     https://issues.apache.org/jira/browse/SQOOP-3310
> 
> 
> Repository: sqoop-trunk
> 
> 
> Description
> -------
> 
> Adds the ability to import data from RDBMS to Apache Kafka
> 
> 
> Diffs
> -----
> 
>   ivy.xml 6be4fa20fbbf1f303c69d86942b1874e18a14afc 
>   ivy/libraries.properties c44b50bc1361bb3a32f47ed2c1b9f291e8c18d05 
>   src/java/org/apache/sqoop/SqoopOptions.java 651cebd69ee7e75d06c75945e3607c4fab7eb11c

>   src/java/org/apache/sqoop/kafka/GenericRecordKafkaTransformer.java PRE-CREATION 
>   src/java/org/apache/sqoop/kafka/JsonKafkaTransformer.java PRE-CREATION 
>   src/java/org/apache/sqoop/kafka/KafkaProduceProcessor.java PRE-CREATION 
>   src/java/org/apache/sqoop/kafka/KafkaUtil.java PRE-CREATION 
>   src/java/org/apache/sqoop/kafka/ProducerRecordTransformer.java PRE-CREATION 
>   src/java/org/apache/sqoop/kafka/ProducerWrapper.java PRE-CREATION 
>   src/java/org/apache/sqoop/manager/SqlManager.java fe997c5f5186b027a7a91002b5544b69393973d0

>   src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java a5962ba44282fc3ae48de23860de0992586e549a

>   src/java/org/apache/sqoop/mapreduce/KafkaImportJob.java PRE-CREATION 
>   src/java/org/apache/sqoop/mapreduce/KafkaImportMapper.java PRE-CREATION 
>   src/java/org/apache/sqoop/tool/BaseSqoopTool.java b02e4fe7fda25c7f8171c7db17d15a7987459687

>   src/java/org/apache/sqoop/tool/ImportTool.java e9920058858653bec7407bf7992eb6445401e813

>   src/test/org/apache/sqoop/kafka/KafkaImportTest.java PRE-CREATION 
>   src/test/org/apache/sqoop/kafka/KafkaTestCase.java PRE-CREATION 
>   src/test/org/apache/sqoop/kafka/KafkaTestUtils.java PRE-CREATION 
>   src/test/org/apache/sqoop/kafka/TestGenericRecordKafkaTransformer.java PRE-CREATION

>   src/test/org/apache/sqoop/kafka/TestJsonKafkaTransformer.java PRE-CREATION 
>   src/test/org/apache/sqoop/kafka/TestKafkaProduceProcessor.java PRE-CREATION 
>   src/test/org/apache/sqoop/kafka/TestKafkaUtil.java PRE-CREATION 
>   src/test/org/apache/sqoop/kafka/TestProducerWrapper.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66475/diff/1/
> 
> 
> Testing
> -------
> 
> Ran unit and 3rd party tests successfully
> 
> 
> Thanks,
> 
> Andras Beni
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message