----------------------------------------------------------- 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 > >
