----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68536/#review208043 -----------------------------------------------------------
Hi Bogi, Thanks for submitting this patch, it fills an important gap in Sqoop's S3 support. Please see my below findings: src/java/org/apache/sqoop/tool/ImportTool.java Lines 1170 (patched) <https://reviews.apache.org/r/68536/#comment291788> Can we somehow simplify and merge these 2 if statements? Ideas: - since IncrementalMode has only 3 possible values checking if options.getIncrementalMode() != SqoopOptions.IncrementalMode.None might be enough - by using org.apache.commons.lang3.StringUtils#contains we could avoid null checks src/test/org/apache/sqoop/tool/TestS3IncrementalImportOptionValidations.java Lines 97 (patched) <https://reviews.apache.org/r/68536/#comment291789> Just for the sake of completeness we could add test case(s) to verify if no exception is thrown when everything is OK. - Szabolcs Vasas On Aug. 28, 2018, 11:56 a.m., Boglarka Egyed wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68536/ > ----------------------------------------------------------- > > (Updated Aug. 28, 2018, 11:56 a.m.) > > > Review request for Sqoop, daniel voros, Fero Szabo, Nguyen Truong, and > Szabolcs Vasas. > > > Bugs: SQOOP-3368 > https://issues.apache.org/jira/browse/SQOOP-3368 > > > Repository: sqoop-trunk > > > Description > ------- > > The current implementation of Sqoop handles HDFS as a default filesystem, > i.e. it creates temporary directories on HDFS in case of incremental append > or merge imports. To make these incremental import use cases work with S3 the > user needs to set the --temporary-rootdir to an S3 location properly. > > > Diffs > ----- > > src/java/org/apache/sqoop/tool/ImportTool.java > 139733732d2a28d171568b9118c98a47a3d2fc50 > > src/test/org/apache/sqoop/tool/TestS3IncrementalImportOptionValidations.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/68536/diff/1/ > > > Testing > ------- > > ant clean test > ./gradlew test > > > Thanks, > > Boglarka Egyed > >
