sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Szabolcs Vasas <vasas.szabo...@gmail.com>
Subject Re: Review Request 59710: SQOOP-3190: Remove dependency on PSQL for postgres direct import
Date Fri, 30 Jun 2017 13:00:54 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message