> On May 29, 2018, 8:56 a.m., Fero Szabo wrote: > > 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.
Thanks for your review Fero. Shall I include it in this patch? Also I found someone had created a script to mock the FTP, I modified somewhat to make it work the way I see the mainframe I work with behaves. I will have to see how integration tests are working now. Maybe running in docker might be a good way to test. > On May 29, 2018, 8:56 a.m., Fero Szabo wrote: > > src/java/org/apache/sqoop/tool/MainframeImportTool.java > > Lines 90 (patched) > > <https://reviews.apache.org/r/62492/diff/11/?file=2031395#file2031395line91> > > > > What is the unit of the buffersize? > > Your example suggests that it's kB. > > Can you please specify it in the description? I've updated the description. > On May 29, 2018, 8:56 a.m., Fero Szabo wrote: > > src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java > > Lines 215 (patched) > > <https://reviews.apache.org/r/62492/diff/11/?file=2031396#file2031396line215> > > > > Just asking out of curiosity, do you know the root cause for this? This is because mainframe ftp doesn't like the command that Apache FTPClient sends for this. I had to find out manually what the command is to send to mainframe to make this work. > On May 29, 2018, 8:56 a.m., Fero Szabo wrote: > > src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java > > Lines 221 (patched) > > <https://reviews.apache.org/r/62492/diff/11/?file=2031396#file2031396line221> > > > > 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. The default appears to be ASCII. - Chris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62492/#review203967 ----------------------------------------------------------- On May 30, 2018, 6:26 a.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62492/ > ----------------------------------------------------------- > > (Updated May 30, 2018, 6:26 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/13/ > > > Testing > ------- > > Unit tests. > > Functional testing on mainframe. > > > Thanks, > > Chris Teoh > >
