sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Abraham Elmahrek" <...@cloudera.com>
Subject Re: Review Request 26948: SQOOP-1523: Sqoop2: Support for PostgreSQL database for hosting the metadata repository
Date Thu, 04 Dec 2014 21:15:01 GMT


> On Dec. 4, 2014, 6:30 p.m., Jarek Cecho wrote:
> > Good job Abe! I've just read the code and I'm still yet to try the changes on real
cluster, but I wanted to share my thoughts early. I have couple of high level questions and
then couple of nits:
> > 
> > 1) Are you also planning to provide functional tests in addition to the structural
ones? Like create link/job and verifying other calls that are prescribed by Repository interface
[1]
> > 
> > 2) Are you planning to provide a documentation on how to use the new repository?
Perhaps a section in the Administration guide talking about different repository implementations?
> > 
> > 3) The tests are creating a lot of tables, but I did not see any clean up. In case
that I've did not simply missed it, we should add it, otherwise test re-execution won't be
that simple.
> > 
> > Links:
> > 1: https://github.com/apache/sqoop/blob/sqoop2/core/src/main/java/org/apache/sqoop/repository/Repository.java
> 
> Jarek Cecho wrote:
>     Please scratch 3), I found the clean up routine :)
> 
> Jarek Cecho wrote:
>     I've give it a spin on my real cluster and I can confirm that it's working just fine,
thank you Abe!

Functional tests are in https://issues.apache.org/jira/browse/SQOOP-1591. Documentation is
dependent on https://issues.apache.org/jira/browse/SQOOP-1680. Can create follow up on this
though. Tests are removing entire schema in PostgresqlTestCase#tearDown. Is there any other
clean up that you think might be useful?


> On Dec. 4, 2014, 6:30 p.m., Jarek Cecho wrote:
> > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java,
lines 83-84
> > <https://reviews.apache.org/r/26948/diff/3/?file=779836#file779836line83>
> >
> >     If we want to use the constant in Lower case, then let's define them as such?
It seems weird to have the contant in upper case and then lowercase it on every use :)
> 
> Jarek Cecho wrote:
>     Actually, I would generalize my comment. When playing with the PostgreSQL repository
on real cluster, I've noticed that PostgreSQL will lower case all table/column names that
are not explicilty quoted. Hence I would suggest to either lower case all the constants or
escape them in the queries - whatever we prefer more :)

In that case, for consistency, would you feel ok with having every thing lower case (table
names, column names, etc.)?


> On Dec. 4, 2014, 6:30 p.m., Jarek Cecho wrote:
> > repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaQuery.java,
lines 23-26
> > <https://reviews.apache.org/r/26948/diff/3/?file=779839#file779839line23>
> >
> >     I see the same very long comment in file PostgresqlSchemaCreateQuery. Let's
perhaps have it in one single file and reference it in the other? I'm afraid that having it
in several places will be harder for maintenance.

https://issues.apache.org/jira/browse/SQOOP-1824


> On Dec. 4, 2014, 6:30 p.m., Jarek Cecho wrote:
> > repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestCase.java,
lines 44-48
> > <https://reviews.apache.org/r/26948/diff/3/?file=779841#file779841line44>
> >
> >     I'm a bit worried about this approach as it will hide configuration issues -
e.g. if the database is down or credentials are missconfigured, the test will still pass and
the fact that we are not testing PostgreSQL repo will be silently ignored.
> >     
> >     Let's perhaps promote those tests into Integration tests similarly as we're
doing in test module [1] and add a flag to enable them?
> >     
> >     Another idea would be to use JUnit categories [2] and it's maven integration
to run them in a special profile/flag.
> >     
> >     I'm happy to take this one into subsequent JIRA as it's more general topic though.
> >     
> >     Links:
> >     1: https://github.com/apache/sqoop/blob/sqoop2/test/pom.xml#L154
> >     2: https://weblogs.java.net/blog/johnsmart/archive/2010/04/25/grouping-tests-using-junit-categories-0
> >     3: http://maven.apache.org/surefire/maven-surefire-plugin/examples/junit.html

Interesting ideas! I've created SQOOP-1591 to address testing of this repository.


> On Dec. 4, 2014, 6:30 p.m., Jarek Cecho wrote:
> > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java,
line 2203
> > <https://reviews.apache.org/r/26948/diff/3/?file=779830#file779830line2203>
> >
> >     Future: Perhaps a suggestion for future, I think that this can be further improved
to simply use PrepareStatement.setObject() to support any type :)

Cool.


- Abraham


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26948/#review63855
-----------------------------------------------------------


On Dec. 2, 2014, 1:48 a.m., Abraham Elmahrek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26948/
> -----------------------------------------------------------
> 
> (Updated Dec. 2, 2014, 1:48 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Bugs: SQOOP-1523
>     https://issues.apache.org/jira/browse/SQOOP-1523
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> commit 0702bddfa69f63f56bfc6909dfe1b3e4f3219c6a
> Author: Abraham Elmahrek <abraham@elmahrek.com>
> Date:   Tue Oct 14 12:05:53 2014 -0700
> 
>     Sqoop2: Postgresql repository
> 
> :100644 100644 5a8e026... 2c5c972... M  core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java
> :100644 100644 f25a29f... 5be6ad9... M  pom.xml
> :100644 100644 e3345c4... 8b308c9... M  repository/pom.xml
> :000000 100644 0000000... 19f67d4... A  repository/repository-postgresql/pom.xml
> :000000 100644 0000000... cdeed35... A  repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepoConstants.java
> :000000 100644 0000000... 872683e... A  repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepoError.java
> :000000 100644 0000000... 93352a6... A  repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java
> :000000 100644 0000000... d1ff898... A  repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaConstants.java
> :000000 100644 0000000... d70dd86... A  repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaQuery.java
> :000000 100644 0000000... 31cc6dc... A  repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestCase.java
> :000000 100644 0000000... a8f4242... A  repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestConnectorHandling.java
> :000000 100644 0000000... 44ffced... A  repository/repository-postgresql/src/test/resources/log4j.properties
> :100644 100644 67baaa5... 8ecdd01... M  server/pom.xml
> 
> 
> Diffs
> -----
> 
>   common-test/pom.xml 9fd671c 
>   common-test/src/main/java/org/apache/sqoop/common/test/db/DatabaseProvider.java 0a12d7b

>   common-test/src/main/java/org/apache/sqoop/common/test/db/PostgreSQLProvider.java d46e01d

>   core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java 4fe1500 
>   pom.xml e6ffc78 
>   repository/pom.xml 8c95c0e 
>   repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java
c278406 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java
6bc5674 
>   repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java
907978f 
>   repository/repository-postgresql/pom.xml PRE-CREATION 
>   repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepoConstants.java
PRE-CREATION 
>   repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepoError.java
PRE-CREATION 
>   repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlRepositoryHandler.java
PRE-CREATION 
>   repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaConstants.java
PRE-CREATION 
>   repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaCreateQuery.java
PRE-CREATION 
>   repository/repository-postgresql/src/main/java/org/apache/sqoop/repository/postgresql/PostgresqlSchemaQuery.java
PRE-CREATION 
>   repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgreSQLTestUtils.java
PRE-CREATION 
>   repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestCase.java
PRE-CREATION 
>   repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/PostgresqlTestUtils.java
PRE-CREATION 
>   repository/repository-postgresql/src/test/java/org/apache/sqoop/repository/postgresql/TestStructure.java
PRE-CREATION 
>   repository/repository-postgresql/src/test/resources/log4j.properties PRE-CREATION 
>   server/pom.xml 1adcca0 
> 
> Diff: https://reviews.apache.org/r/26948/diff/
> 
> 
> Testing
> -------
> 
> Can start Sqoop2 server.
> Ran integration tests with PostgreSQL. All pass except partitioning on a boolean column.
> Also:
>  482  disable link -l 1
>  483  show link --all
>  484  show link
>  485  enable link -l 1
>  486  show link
>  487  show link --all
>  488  show job
>  489  show job --all
>  490  show connector
>  491  show connector --all
>  492  help
>  493  status job --jid 1
>  494  history
>  495  start job --jid 1
>  496  clone job --jid 1
>  497  delete job --jid 2
>  498  status job --jid 1
>  499  history
> 
> 
> Thanks,
> 
> Abraham Elmahrek
> 
>


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