----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27085/#review58243 -----------------------------------------------------------
core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment99194> `LOG.isDebugEnabled()` is this really necessary? I'd suggest use parameterized logging rather than add if a if-clausel. http://www.slf4j.org/faq.html#logging_performance core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment99195> For better code understanding, maybe better group `updateConnectorStatement` `deleteInput` and `deleteConfig` in three blocks? deleteInput = conn.prepareStatement(STMT_DELETE_INPUTS_FOR_CONFIGURABLE); deleteInput.setLong(1, mConnector.getPersistenceId()); deleteInput.executeUpdate(); deleteConfig = conn.prepareStatement(STMT_DELETE_CONFIGS_FOR_CONFIGURABLE); deleteConfig.setLong(1, mConnector.getPersistenceId()); deleteConfig.executeUpdate(); core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment99196> Can we use `closeStatements()` as other classes do? core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment99198> Nice to have: `return submission != null && submission.getStatus().isRunning();` core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment99199> How about create local `ResultSet` (and close it locally) instead creating one `ResultSet` out of the for-block? please search for similar occurences core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment99202> no close resultset? (please search for similar occurences) core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment99206> General question, if check null is required, then please add the check to all code occurences, otherwise please remove unused null check. core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java <https://reviews.apache.org/r/27085/#comment99208> General question: please make methods static if it is possible. - Qian Xu On Oct. 24, 2014, 1:13 a.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27085/ > ----------------------------------------------------------- > > (Updated Oct. 24, 2014, 1:13 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-158 > https://issues.apache.org/jira/browse/SQOOP-158 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > commit 370f476b1834c390993b31007b0fc7e1a184b891 > 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 7d78826... e41cd6c... M > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java > :000000 100644 0000000... a7ebd50... A > core/src/main/java/org/apache/sqoop/repository/JdbcRepositorySchemaConstants.java > :000000 100644 0000000... eb05567... A > core/src/main/java/org/apache/sqoop/repository/JdbcRepositorySchemaQuery.java > :100644 100644 f684e85... 097a185... M > core/src/main/java/org/apache/sqoop/repository/RepositoryError.java > :100644 100644 aad219e... 769544b... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java > :100644 100644 633e9df... 6764b14... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > :100644 100644 ce1830e... ff8962c... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java > :100644 100644 85140d5... aab1e95... M > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java > :100644 100644 37343d3... 9bcf3c8... M > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java > > > Diffs > ----- > > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java > 7d78826 > > core/src/main/java/org/apache/sqoop/repository/JdbcRepositorySchemaConstants.java > PRE-CREATION > > core/src/main/java/org/apache/sqoop/repository/JdbcRepositorySchemaQuery.java > PRE-CREATION > core/src/main/java/org/apache/sqoop/repository/RepositoryError.java f684e85 > > 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 > 633e9df > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java > ce1830e > > 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 > 37343d3 > > Diff: https://reviews.apache.org/r/27085/diff/ > > > Testing > ------- > > mvn clean verify + start sqoop server without issues. > > > Thanks, > > Abraham Elmahrek > >
