----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62492/#review203967 -----------------------------------------------------------
Hi Chris, I had a quick glance at your patch. Mainly, it seems to be fine for me, with some minor issues. I'm trying to get new Sqoop features documented. Since this is a new feature, can you please add documentation to the project? Just a couple of sentences and one or two examples should suffice! The file to modify is located under src/docs. (It's probably src/docs/user/import-mainframe.txt.) It's unclear to me how I could better test it. I wonder how difficult would it be for me to emulate a mainframe on a virtual machine and somehow integrate that with sqoop. Certainly sounds like a lot of effort! If more of these patches are coming, than we might benefit from an integration test environment like this. src/java/org/apache/sqoop/tool/MainframeImportTool.java Lines 90 (patched) <https://reviews.apache.org/r/62492/#comment286334> What is the unit of the buffersize? Your example suggests that it's kB. Can you please specify it in the description? src/java/org/apache/sqoop/tool/MainframeImportTool.java Lines 218 (patched) <https://reviews.apache.org/r/62492/#comment286324> Looks like incorrect parenthesis to me. Shouldn't it be? if (SqoopOptions.FileLayout.BinaryFile.equals(options.getFileLayout()) && ( options.getMainframeInputDatasetName() == null || options.getMainframeInputDatasetName().equals("") ) ) { ... Anyway, I recommend using org.apache.commons.lang3.StringUtils.isEmpty() to check if a String is null or empty src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java Lines 213 (patched) <https://reviews.apache.org/r/62492/#comment286328> Why the extra parenthesis around (transfermode)? :) src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java Lines 215 (patched) <https://reviews.apache.org/r/62492/#comment286331> Just asking out of curiosity, do you know the root cause for this? src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java Lines 221 (patched) <https://reviews.apache.org/r/62492/#comment286332> Would it make sense to rather throw an exception here and force the user to specify the encoding? Though I'm not too familiar with FTP... Does it always default to ASCII? In that case, I admit that it would make sense to use this as the default. - Fero Szabo On May 29, 2018, 8:05 a.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62492/ > ----------------------------------------------------------- > > (Updated May 29, 2018, 8:05 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-3224 > https://issues.apache.org/jira/browse/SQOOP-3224 > > > Repository: sqoop-trunk > > > Description > ------- > > Added --as-binaryfile and --buffersize to support FTP transfer mode switching. > > > Diffs > ----- > > src/java/org/apache/sqoop/SqoopOptions.java d9984af3 > src/java/org/apache/sqoop/mapreduce/DataDrivenImportJob.java a5962ba4 > src/java/org/apache/sqoop/mapreduce/KeyRecordWriters.java PRE-CREATION > src/java/org/apache/sqoop/mapreduce/RawKeyTextOutputFormat.java fec34f21 > > src/java/org/apache/sqoop/mapreduce/mainframe/AbstractMainframeDatasetImportMapper.java > PRE-CREATION > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java > ea54b07f > > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetBinaryImportMapper.java > PRE-CREATION > > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetBinaryRecord.java > PRE-CREATION > > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetFTPRecordReader.java > 1f78384b > > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeDatasetImportMapper.java > 0b7b5b85 > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeImportJob.java > 7e975c7b > src/java/org/apache/sqoop/tool/BaseSqoopTool.java 783651a4 > src/java/org/apache/sqoop/tool/ImportTool.java ee79d8b7 > src/java/org/apache/sqoop/tool/MainframeImportTool.java 8883301d > src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java 95bc0ecb > > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetBinaryRecord.java > PRE-CREATION > > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeDatasetFTPRecordReader.java > 3547294f > src/test/org/apache/sqoop/tool/TestMainframeImportTool.java 0b0c6c34 > src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 90a85194 > > > Diff: https://reviews.apache.org/r/62492/diff/11/ > > > Testing > ------- > > Unit tests. > > Functional testing on mainframe. > > > Thanks, > > Chris Teoh > >
