----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67408/#review208552 -----------------------------------------------------------
Hi Chris, Thank you for submitting this patch, I have successfully executed both the unit and third party test pack. My understanding of the GDG datasets is limited can you please explain a bit more what the problem is this patch solves? You could give an example which did not work properly previously but works correctly with this patch. I have left some code style related suggestions please take a look. I can see a big improvement since your last patches, I think you follow much more best practices now, which is great! So keep up the good work! :) src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java Line 34 (original), 34 (patched) <https://reviews.apache.org/r/67408/#comment292573> Just to avoid future refactoring traps can you put MainframeFTPFileGdgEntryParser.class.getName() here instead of the string constant? src/java/org/apache/sqoop/mapreduce/mainframe/MainframeFTPFileGdgEntryParser.java Lines 54 (patched) <https://reviews.apache.org/r/67408/#comment292577> It seems that this constructor is never used, do we need it? I am not sure it would make sense instantiating a MainframeFTPFileGdgEntryParser with a different regexp. src/java/org/apache/sqoop/mapreduce/mainframe/MainframeFTPFileGdgEntryParser.java Lines 80 (patched) <https://reviews.apache.org/r/67408/#comment292578> It might be a bit cleaner to extract this string into a constant. src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeFTPFileGdgEntryParser.java Lines 49 (patched) <https://reviews.apache.org/r/67408/#comment292580> I think these fields can all be non-static members. You can convert the setUpBeforClass into a @Before method and initialize the parser field as well in that method (so you can remove the initialization from the individual test methods). src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeFTPFileGdgEntryParser.java Lines 52 (patched) <https://reviews.apache.org/r/67408/#comment292579> You can use the diamond operator here: new ArrayList<> src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeFTPFileGdgEntryParser.java Lines 84 (patched) <https://reviews.apache.org/r/67408/#comment292581> Instead of this little bit obscure for-loop you could use a nice Java 8 syntax here for counting the parsed entries: long count = listing.stream() .map(parser::parseFTPEntry) .filter(Objects::nonNull) .count(); - Szabolcs Vasas On June 1, 2018, 4:46 a.m., Chris Teoh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67408/ > ----------------------------------------------------------- > > (Updated June 1, 2018, 4:46 a.m.) > > > Review request for Sqoop. > > > Repository: sqoop-trunk > > > Description > ------- > > Mainframe FTP listing for GDG should filter out non-GDG datasets in a > heterogeneous listing as it grabs the last file and in the case where there > are other datasets mixed in, the latest file may not be the desired dataset. > > > Diffs > ----- > > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeConfiguration.java > 9d6a2fe7 > > src/java/org/apache/sqoop/mapreduce/mainframe/MainframeFTPFileGdgEntryParser.java > PRE-CREATION > src/java/org/apache/sqoop/util/MainframeFTPClientUtils.java 654721e3 > > src/test/org/apache/sqoop/mapreduce/mainframe/TestMainframeFTPFileGdgEntryParser.java > PRE-CREATION > src/test/org/apache/sqoop/util/TestMainframeFTPClientUtils.java 90a85194 > > > Diff: https://reviews.apache.org/r/67408/diff/2/ > > > Testing > ------- > > Unit tests. Integration testing locally on developer machine. > > > Thanks, > > Chris Teoh > >
