----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62653/#review186652 -----------------------------------------------------------
Hi Chris, Thank you for improving the patch and the description of the datasets! I have left few more comments and I can see that there are still some whitespace changes (they are shown by ReviewBoard as well) can you please fix those as well? I have run the unit tests and I can see that TestMainframeDatasetInputFormat is failing. Can you please check it? Thanks, Szabolcs src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java Line 83 (original), 82 (patched) <https://reviews.apache.org/r/62653/#comment263429> If dsType can be null then this might throw a NullPointerException. By changing this to !MainframeConfiguration.MAINFRAME_INPUT_DATASET_TYPE_PARTITIONED.equals(dsType) we could eliminate it. src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java Lines 342 (patched) <https://reviews.apache.org/r/62653/#comment263442> I think you could use the Mockito.verify() method instead of exception throwing. See my other comment below. src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java Line 355 (original), 358 (patched) <https://reviews.apache.org/r/62653/#comment263431> Using the methods of the Assert class is considered to be a best practice since the assert keyword only works if the -ea parameter is specified for the JVM. You can add the verify line here as well: verify(mockFTPClient).listFiles(); - Szabolcs Vasas On Sept. 28, 2017, 9:34 p.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62653/ > ----------------------------------------------------------- > > (Updated Sept. 28, 2017, 9:34 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-3225 > https://issues.apache.org/jira/browse/SQOOP-3225 > > > Repository: sqoop-trunk > > > Description > ------- > > There were some cases where the FTP listing parser was not used for mainframe > datasets, only the default was used. This caused some datasets to not be seen > by Sqoop as the default parser couldn't find them. This patch addresses this > behaviour where it is used to parse FTP listings for sequential and gdg > datasets on disk and tape, only partitioned datasets are excluded as their > FTP listing looks very different and is handled by the default FTP listing > parser. > > > Diffs > ----- > > src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java f61b9838 > src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java d87c75df > > > Diff: https://reviews.apache.org/r/62653/diff/4/ > > > Testing > ------- > > Unit testing. > Functional testing on a real mainframe with a previous combined patch. > > > Thanks, > > Chris Teoh > >
