> 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! > > Abraham Elmahrek wrote: > 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?
Thanks for the response Abe! Fair enough, I'm fine with having the functional tests committed soon after as this separate and new component. I'm assuming that the tests will be added soon after this get in, correct? The documentation JIRA SQOOP-1680 seems to be covering more an APIs for someone who is going to create new repository rather then instructing end user how to configure the repository. I know that all of the required properties are stored in "sqoop.properties" file and one just need to update them, but I was thinking about having a repository section in our admin guide that will list existing repositories and example configuration snipnets how to enable them. > 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 :) > > Abraham Elmahrek wrote: > In that case, for consistency, would you feel ok with having every thing > lower case (table names, column names, etc.)? I honestly don't have strong preference whether the names should be lower or upper case. I just feel that we should be consistent in what we have in the constants and what is actually used in the database. > 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. > > Abraham Elmahrek wrote: > https://issues.apache.org/jira/browse/SQOOP-1824 I'm fine with having the schema inside java file, because it's internal to the repository implementation. End user (and admin) should never go directly to the database and do changes there. I'm just a bit worried that keeping the copy in two places will get easily out of sync. > 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 > > Abraham Elmahrek wrote: > Interesting ideas! I've created SQOOP-1591 to address testing of this > repository. I've copy&pasted my comment to the SQOOP-1591 as that JIRA seemed to be missing all the details I've pointed out here :) - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26948/#review63855 ----------------------------------------------------------- On Dec. 4, 2014, 9:18 p.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26948/ > ----------------------------------------------------------- > > (Updated Dec. 4, 2014, 9:18 p.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 <[email protected]> > 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 > 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-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/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 > >
