> On Oct. 9, 2014, 12:21 a.m., Jarek Cecho wrote: > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java, > > line 521 > > <https://reviews.apache.org/r/26097/diff/5/?file=716198#file716198line521> > > > > 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.
You're right. Thanks Jarcec. > On Oct. 9, 2014, 12:21 a.m., Jarek Cecho wrote: > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java, > > line 536 > > <https://reviews.apache.org/r/26097/diff/5/?file=716198#file716198line536> > > > > 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 :) Had a very similar thought actually. +1 to this. > On Oct. 9, 2014, 12:21 a.m., Jarek Cecho wrote: > > common/src/main/java/org/apache/sqoop/common/SupportedDirections.java, line > > 69 > > <https://reviews.apache.org/r/26097/diff/5/?file=716195#file716195line69> > > > > 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) I'm going to yank this code since I'm no longer using it. Thanks for identifying this. > On Oct. 9, 2014, 12:21 a.m., Jarek Cecho wrote: > > common/src/main/java/org/apache/sqoop/common/SupportedDirections.java, > > lines 99-116 > > <https://reviews.apache.org/r/26097/diff/5/?file=716195#file716195line99> > > > > Nit: Seems quite complex logic for something as simple as comparting > > two boolean values. Isn't there an easier way? I'm going to yank this code since I'm no longer using it. Thanks for identifying this. - Abraham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26097/#review55908 ----------------------------------------------------------- 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 > >
