sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Ang <alexan...@gmail.com>
Subject Re: Review Request 59710: SQOOP-3190: Remove dependency on PSQL for postgres direct import
Date Fri, 30 Jun 2017 17:19:48 GMT


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


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