> On June 30, 2017, 1 p.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java > > Line 348 (original), 241 (patched) > > <https://reviews.apache.org/r/59710/diff/1/?file=1736285#file1736285line355> > > > > This method seems to do too many things: it validates the options, > > builds the connection and copies the data. Can we refactor it into more > > smaller methods to make it more readable and maintainable?
Refactored. > On June 30, 2017, 1 p.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java > > Line 429 (original), 276 (patched) > > <https://reviews.apache.org/r/59710/diff/1/?file=1736285#file1736285line436> > > > > Can we use this class from org.apache.sqooop package? Refactored. > On June 30, 2017, 1 p.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java > > Lines 281 (patched) > > <https://reviews.apache.org/r/59710/diff/1/?file=1736285#file1736285line444> > > > > The password is not set in the Configuration and because of this some > > test cases in PostgresqlImportTest fail. Added the password into the config. > On June 30, 2017, 1 p.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java > > Lines 283 (patched) > > <https://reviews.apache.org/r/59710/diff/1/?file=1736285#file1736285line446> > > > > I am not sure why the JobConf object is needed here. The > > DBConfiguration constructor seems to expect a Configuration object, so conf > > could be passed as a parameter, couldn't it? DBConfiguration expects a org.apache.hadoop.mapred.JobConf and not org.apache.hadoop.conf.Configuration. Without the wrapper, ClassCastException will be thrown. > On June 30, 2017, 1 p.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java > > Line 438 (original), 297 (patched) > > <https://reviews.apache.org/r/59710/diff/1/?file=1736285#file1736285line460> > > > > I know it was in the original code as well but could we choose a more > > expressive name instead of the one character variable name? Updated in the new diff. > On June 30, 2017, 1 p.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java > > Line 505 (original), 312 (patched) > > <https://reviews.apache.org/r/59710/diff/1/?file=1736285#file1736285line528> > > > > I think the string encoding should be specified here to avoid using > > defaults. Could you advise where the encoding can be retrieved or I should default it to UTF-8? Updated to UTF-8 in the new diff. > On June 30, 2017, 1 p.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java > > Line 506 (original), 313 (patched) > > <https://reviews.apache.org/r/59710/diff/1/?file=1736285#file1736285line529> > > > > Don't we need to apply the record delimiter here (see line 135 in the > > original file)? No. Applying the record delimiter will create additional row and fail the unit test. > On June 30, 2017, 1 p.m., Szabolcs Vasas wrote: > > src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java > > Lines 337 (patched) > > <https://reviews.apache.org/r/59710/diff/1/?file=1736285#file1736285line564> > > > > This line is redundant, as the finally block will be called even if we > > throw an exception. Removed. - Alex ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59710/#review177994 ----------------------------------------------------------- On June 30, 2017, 5:17 p.m., Alex Ang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59710/ > ----------------------------------------------------------- > > (Updated June 30, 2017, 5:17 p.m.) > > > Review request for Sqoop and Szabolcs Vasas. > > > Bugs: SQOOP-3190 > https://issues.apache.org/jira/browse/SQOOP-3190 > > > Repository: sqoop-trunk > > > Description > ------- > > Current Postgres direct import requires the installation of the psql utility > on all hadoop nodes. However, due to system constraints, this may not always > be possible. > It should be noted that Postgres provides a Java API which can execute the > same copy command as the psql utility. As such, it would be more ideal to > remove the psql utility dependency and switch to the Postgres Java API. > > > Diffs > ----- > > src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java 63b0704 > > > Diff: https://reviews.apache.org/r/59710/diff/2/ > > > Testing > ------- > > Tested with PostgresqlImportTest and passed all tests. > > > Thanks, > > Alex Ang > >
