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 Thu, 06 Jul 2017 14:03:33 GMT


> 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.
> 
> Alex Ang wrote:
>     Added the password into the config.
> 
> Szabolcs Vasas wrote:
>     I think adding the password is a bit trickier than just adding to the conf, PostgresqlImportTest
still fails with the same error on my side.
> 
> Alex Ang wrote:
>     I have some difficulty setting the postgres db where the original code and PostgresqlExportTest
also fail due to  org.postgresql.util.PSQLException: FATAL: password authentication failed
for user "sqooptest". 
>     This would take some time as I have to sort out the environment. 
>     Need to fix this issue before I can post the new patch.
>     
>     Pls note that in the PostgresqlImportTest and the cwiki (https://cwiki.apache.org/confluence/display/SQOOP/Setting+up+Sqoop+1#SettingupSqoop1-SettingupPostgreSQL),
the environment setup uses trust where password are not required. 
>     Would appreciate if you are able to guide me to the correct setup.
>     FYI, I had success using --password-file in another setup.

Sorry, I haven't taken into consideration that https://issues.apache.org/jira/browse/SQOOP-3197
is not yet committed. A few weeks ago I have realized that PostgreSQL third party tests cannot
handle database passwords, most probably this was the reason the Wiki suggested creating a
user without password. But this need to be fixed since users have passwords in production.
I will try to have SQOOP-3197 committed then you can verify your code change. Without the
fix you could test it manually. If you want to specify a password for a new user you can use
this command:

CREATE USER sqooptest with PASSWORD 'sqooptest';


> 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?
> 
> Alex Ang wrote:
>     DBConfiguration expects a org.apache.hadoop.mapred.JobConf and not org.apache.hadoop.conf.Configuration.
Without the wrapper, ClassCastException will be thrown.
> 
> Szabolcs Vasas wrote:
>     Are you sure you use the trunk branch? On trunk I can see this constructor: http://github.mtv.cloudera.com/CDH/sqoop/blob/cdh5-1.4.6/src/java/org/apache/sqoop/mapreduce/db/DBConfiguration.java#L266
> 
> Alex Ang wrote:
>     I am using the trunk from https://github.com/apache/sqoop 
>     Is there somewhere else I should be getting the source from?

Sorry, I have posted a wrong URL, but I use the same repo as you. Meanwhile I have realized
what you mean, the constructor parameter is defined as Configuration but later it is casted
to JobConf.


> 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.
> 
> Alex Ang wrote:
>     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.
> 
> Szabolcs Vasas wrote:
>     UTF-8 seems to be used in org.apache.sqoop.mapreduce.postgresql.PostgreSQLCopyExportMapper
too but I think you should check what is the encoding used by the psql command because some
users might rely on it.
> 
> Alex Ang wrote:
>     I have defaulted the encoding to follow the database as per the postgres CopyManager
code (line 94 https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/main/java/org/postgresql/copy/CopyManager.java).

>     Pls let me know if this is acceptable.

I am not sure what you mean, but I think it is a good idea to use the Encoding object to encode
the byte to strings. And I guess you should initialize it like this: this.encoding = connection.getEncoding();


- Szabolcs


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