----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59710/#review177994 -----------------------------------------------------------
Hi Alex, Thank you for this patch, this improvement would be really valuable! However there are a few things I think need to be fixed before it can be committed, please find my comments below. My general note is that please upload your patch without reformatting. The unrelated reformats makes it harder to handle the patch. Regards, Szabolcs src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 178 (original), 89 (patched) <https://reviews.apache.org/r/59710/#comment251727> Reformat. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 181 (original), 92 (patched) <https://reviews.apache.org/r/59710/#comment251728> Reformat. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 199 (original), 111 (patched) <https://reviews.apache.org/r/59710/#comment251729> Reformat. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 208 (original), 120 (patched) <https://reviews.apache.org/r/59710/#comment251730> Reformat. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 226 (original), 138 (patched) <https://reviews.apache.org/r/59710/#comment251731> Reformat. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 233 (original), 143 (patched) <https://reviews.apache.org/r/59710/#comment251732> Reformat. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 251 (original), 158 (patched) <https://reviews.apache.org/r/59710/#comment251733> Reformat. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 259 (original), 166 (patched) <https://reviews.apache.org/r/59710/#comment251734> Reformat. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 267 (original), 174 (patched) <https://reviews.apache.org/r/59710/#comment251735> Reformat. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 298 (original), 205 (patched) <https://reviews.apache.org/r/59710/#comment251736> Reformat. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 345 (original), 238 (patched) <https://reviews.apache.org/r/59710/#comment251737> Reformat. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 348 (original), 241 (patched) <https://reviews.apache.org/r/59710/#comment254004> 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? src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 368 (original), 261 (patched) <https://reviews.apache.org/r/59710/#comment251738> Reformat. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 429 (original), 276 (patched) <https://reviews.apache.org/r/59710/#comment251739> Can we use this class from org.apache.sqooop package? src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Lines 281 (patched) <https://reviews.apache.org/r/59710/#comment254009> The password is not set in the Configuration and because of this some test cases in PostgresqlImportTest fail. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Lines 283 (patched) <https://reviews.apache.org/r/59710/#comment254006> 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? src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 438 (original), 297 (patched) <https://reviews.apache.org/r/59710/#comment251740> 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? src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 505 (original), 312 (patched) <https://reviews.apache.org/r/59710/#comment254007> I think the string encoding should be specified here to avoid using defaults. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 506 (original), 313 (patched) <https://reviews.apache.org/r/59710/#comment254008> Don't we need to apply the record delimiter here (see line 135 in the original file)? src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Lines 337 (patched) <https://reviews.apache.org/r/59710/#comment254005> This line is redundant, as the finally block will be called even if we throw an exception. src/java/org/apache/sqoop/manager/DirectPostgresqlManager.java Line 584 (original), 387 (patched) <https://reviews.apache.org/r/59710/#comment251741> Reformat. - Szabolcs Vasas On June 15, 2017, 3:03 a.m., Alex Ang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59710/ > ----------------------------------------------------------- > > (Updated June 15, 2017, 3:03 a.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/1/ > > > Testing > ------- > > Tested with PostgresqlImportTest and passed all tests. > > > Thanks, > > Alex Ang > >
