----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26097/#review55908 -----------------------------------------------------------
common/src/main/java/org/apache/sqoop/common/SupportedDirections.java <https://reviews.apache.org/r/26097/#comment96278> I think that we should catch IllegalArgumentException that can be thrown here per the java docs: http://docs.oracle.com/javase/7/docs/api/java/lang/Enum.html#valueOf(java.lang.Class,%20java.lang.String) common/src/main/java/org/apache/sqoop/common/SupportedDirections.java <https://reviews.apache.org/r/26097/#comment96281> Nit: Seems quite complex logic for something as simple as comparting two boolean values. Isn't there an easier way? repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java <https://reviews.apache.org/r/26097/#comment96282> Isn't the map other way around that the comment is suggesting? It shoudl be "Direction -> direction id" rather then "direction id -> Direction", right? repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java <https://reviews.apache.org/r/26097/#comment96283> Do we have to call the prepare statement every time in the loop? Can't we just create it once and reuse it? repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java <https://reviews.apache.org/r/26097/#comment96284> Btw this code is "leaking" statements - we are using one variable in a loop where we are on every iteration assigning new prepared statement without closing the previous one. We're closing only the last created statement via this finally block. I think that we should really bumb creation of the statement outside of the loop. repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java <https://reviews.apache.org/r/26097/#comment96285> Nit: Perhaps an idea for future JIRA - let's kick all the upgrade code to a separate class. This handler getting really out of whack with a lot of code doing various unrelated stuff :) - Jarek Cecho On Oct. 8, 2014, 11:42 p.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26097/ > ----------------------------------------------------------- > > (Updated Oct. 8, 2014, 11:42 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1545 > https://issues.apache.org/jira/browse/SQOOP-1545 > > > Repository: sqoop-SQOOP-1367 > > > Description > ------- > > commit 5cc9f9cd87ef44ff66cc53ad3f0ca38e3ef3d21d > Author: Abraham Elmahrek <[email protected]> > Date: Wed Sep 24 23:20:48 2014 -0700 > > SQOOP-1545: Sqoop2: From/To: Add supported directions to Repository > > :100644 100644 25ba276... 315b60f... M > common/src/main/java/org/apache/sqoop/common/SupportedDirections.java > :100644 100644 4fbaf82... 4f0cdd6... M > common/src/test/java/org/apache/sqoop/common/TestSupportedDirections.java > :100644 100644 5dd7970... de64aa4... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > :100644 100644 58eed2d... 971ef90... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java > :100644 100644 ad42901... 08b1afc... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java > :100644 100644 998f5b7... 6aa28c7... M > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java > :100644 100644 8a5823d... 9ad429d... M > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java > :100644 100644 2ba75b4... 7310a1f... M > shell/src/main/java/org/apache/sqoop/shell/ShowConnectorFunction.java > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/common/SupportedDirections.java > 25ba276 > common/src/test/java/org/apache/sqoop/common/TestSupportedDirections.java > 4fbaf82 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java > cc31d06 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > 73d8387 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java > ad749ed > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java > 478afe2 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java > 2da084f > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java > a0e8b91 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java > 0752923 > shell/src/main/java/org/apache/sqoop/shell/ShowConnectorFunction.java > 09fb195 > spi/src/main/java/org/apache/sqoop/connector/spi/SqoopConnector.java > 849f091 > > Diff: https://reviews.apache.org/r/26097/diff/ > > > Testing > ------- > > mvn test > > > Thanks, > > Abraham Elmahrek > >
