----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27085/#review59359 -----------------------------------------------------------
Awesome stuff. I think it will make life a lot better down the road. It also makes future refactoring easier since most logic is in one place now. repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment100627> Is this an inherited method? If so, any reason we can't inherit docs? repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment100625> This is specific for Derby - it won't work on any other DB AFAIK. Lets move it out of common. repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment100628> docs? repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment100629> docs? (can be separate jira if docs never existed) repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment100630> inherit docs? repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment100631> docs? repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment100632> docs? repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment100633> inherit docs? repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryInsertUpdateDeleteSelectQuery.java <https://reviews.apache.org/r/27085/#comment100634> Perhaps we want to separate this farther into DML (insert, update, delete) and queries (that modify nothing)? I'm not sure if it will make things more or less readable, would like to hear your thoughts on this. repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java <https://reviews.apache.org/r/27085/#comment100636> I'm wondering if DB specific error messages should contain text that indicate the DB. repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment100637> runQuery is underdocumented, which makes the changes challenging to review. Maybe add few lines on what "args" are expected and how they'll be used. repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment100638> Not this patch, but I can't help commenting: ResultSet should have a method for getting the number of rows returned/updated. No need to count. repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment100639> This looks fairly generic after the refactoring. Think it can be moved up the hierarchy? * I'd like some consistency around where we document the methods. And I think since all those methods are inherited, the docs should go in super. We can have a separate JIRA for all the classes where documentation does not already exist. - Gwen Shapira On Oct. 28, 2014, 9:04 p.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27085/ > ----------------------------------------------------------- > > (Updated Oct. 28, 2014, 9:04 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1589 > https://issues.apache.org/jira/browse/SQOOP-1589 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > commit 07f1441c6bb1a99f8e2ef81c89d2b4752fc5d76d > Author: Abraham Elmahrek <[email protected]> > Date: Wed Oct 22 18:30:28 2014 -0700 > > SQOOP-1589: Sqoop2: Create common constants, error codes, and queries > > :100644 100644 f25a29f... 29de641... M pom.xml > :100644 100644 e3345c4... 8c95c0e... M repository/pom.xml > :000000 100644 0000000... 37378c6... A repository/repository-common/pom.xml > :000000 100644 0000000... 6d2c926... A > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryError.java > :000000 100644 0000000... 3198b0f... A > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java > :000000 100644 0000000... cf3df00... A > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryInsertUpdateDeleteSelectQuery.java > :000000 100644 0000000... 173dcb8... A > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositorySchemaConstants.java > :100644 100644 6ad6d64... 9be96db... M repository/repository-derby/pom.xml > :100644 100644 aad219e... 769544b... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java > :100644 100644 7f19c28... 726d4ab... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > :100644 100644 02b11fc... c159183... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaInsertUpdateDeleteSelectQuery.java > :100644 100644 85140d5... 9a64ad9... M > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java > :100644 100644 37343d3... b96b2d7... M > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java > :100644 100644 67baaa5... 090a0b1... M server/pom.xml > :100644 100644 7a80710... 954fc68... M test/pom.xml > > > Diffs > ----- > > pom.xml f25a29f > repository/pom.xml e3345c4 > repository/repository-common/pom.xml PRE-CREATION > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryError.java > PRE-CREATION > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryHandler.java > PRE-CREATION > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositoryInsertUpdateDeleteSelectQuery.java > PRE-CREATION > > repository/repository-common/src/main/java/org/apache/sqoop/repository/common/CommonRepositorySchemaConstants.java > PRE-CREATION > repository/repository-derby/pom.xml 6ad6d64 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java > aad219e > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > 514b5ac > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaInsertUpdateDeleteSelectQuery.java > c894d06 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java > 85140d5 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java > dabb08b > server/pom.xml 67baaa5 > test/pom.xml 7a80710 > > Diff: https://reviews.apache.org/r/27085/diff/ > > > Testing > ------- > > mvn clean verify + start sqoop server without issues. > > > Thanks, > > Abraham Elmahrek > >
