----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24957/#review51647 -----------------------------------------------------------
Good work Abe! I have couple of high level notes below, but otherwise it looks good. I'll try to do some real-cluster testing tomorrow. 1) Do you think that it would make sense to write tests that will test the upgrade procedure? Just to ensure that we're covered there. common/src/main/java/org/apache/sqoop/model/FormUtils.java <https://reviews.apache.org/r/24957/#comment90137> Super nit: Let's handle this independent change separately? common/src/main/java/org/apache/sqoop/model/MConnection.java <https://reviews.apache.org/r/24957/#comment90138> This doesn't seem correct based on the constructor's usage in the "clone" method that is not expecting that new object will have the persistance ID set. common/src/main/java/org/apache/sqoop/model/MJob.java <https://reviews.apache.org/r/24957/#comment90139> This doesn't seem correct based on the constructor's usage in the "clone" method that is not expecting that new object will have the persistance ID set. connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcValidator.java <https://reviews.apache.org/r/24957/#comment90140> Can we skip this change? I'll be removing this class shortly in SQOOP-1444. connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsMetadataUpgrader.java <https://reviews.apache.org/r/24957/#comment90141> Future note: I think that we should do some clean up in the MetadataUpgrader and surroundings. Probably get rid of the MConnectionForms and MJobForms classes as they are pretty much implementing the same and I no longer see why would we need two classes for the same purpose. And then to create separate APIs to upgrade Connection, JobFrom and JobTo. core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java <https://reviews.apache.org/r/24957/#comment90142> Javadocs please. core/src/main/java/org/apache/sqoop/repository/Repository.java <https://reviews.apache.org/r/24957/#comment90143> Let's comment the validations out for now as they are not useful anyway and let's log a sub-task for SQOOP-1439 to fix that independently on this ticket? core/src/main/java/org/apache/sqoop/repository/Repository.java <https://reviews.apache.org/r/24957/#comment90144> Let's comment the validations out for now as they are not useful anyway and let's log a sub-task for SQOOP-1439 to fix that independently on this ticket? repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java <https://reviews.apache.org/r/24957/#comment90145> Let's keep the changelog of what has changed? repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java <https://reviews.apache.org/r/24957/#comment90146> Let's put javadoc describing why we need this? repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java <https://reviews.apache.org/r/24957/#comment90147> Just verifying logic, you're putting this table back to the original form (prior from/to refactoring) so that you can change it later via upgrade, right? repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java <https://reviews.apache.org/r/24957/#comment90148> Hehe, nice :) repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java <https://reviews.apache.org/r/24957/#comment90149> Version 4 upgrade (from 3 to 4), right? Jarcec - Jarek Cecho On Aug. 27, 2014, 1:03 a.m., Abraham Elmahrek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24957/ > ----------------------------------------------------------- > > (Updated Aug. 27, 2014, 1:03 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1374 > https://issues.apache.org/jira/browse/SQOOP-1374 > > > Repository: sqoop-SQOOP-1367 > > > Description > ------- > > commit 3e4abe598f2df1d109a4401f0961b91f229b0be1 > Author: Abraham Elmahrek <[email protected]> > Date: Wed Aug 6 15:26:39 2014 -0700 > > SQOOP-1374: From/To: Metadata upgrade > > :100644 100644 d9666c8... a5399fd... M > common/src/main/java/org/apache/sqoop/model/FormUtils.java > :100644 100644 e5a4fb8... 8336fb7... M > common/src/main/java/org/apache/sqoop/model/MConnection.java > :100644 100644 11839fc... 6d3fe4b... M > common/src/main/java/org/apache/sqoop/model/MJob.java > :100644 100644 2b12009... cbe72f6... M > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnectorMetadataUpgrader.java > :100644 100644 eea86b2... 5447623... M > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcValidator.java > :100644 100644 557091e... 883636c... M > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnector.java > :000000 100644 0000000... 3e51e38... A > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsMetadataUpgrader.java > :100644 100644 b92ff4d... db6f579... M > core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java > :000000 100644 0000000... 4b93c2a... A > core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java > :100644 100644 9b64661... fa119a5... M > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java > :100644 100644 e9c32e0... f75b5b3... M > core/src/main/java/org/apache/sqoop/repository/Repository.java > :100644 100644 030dde7... 41c9bcd... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java > :100644 100644 88be9fb... 1f930a6... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > :100644 100644 1a77360... 1f030ea... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java > :100644 100644 e5bb2e7... 1903b26... M > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java > :100644 100644 f603cc1... 4f0b139... M > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java > :100644 100644 bdd3c05... f9e9217... M > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectionHandling.java > :100644 100644 54ae726... 745e128... M > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java > :100644 100644 8b3326d... 006ec9c... M > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestFrameworkHandling.java > :100644 100644 5d3807d... 15f9539... M > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInputTypes.java > :100644 100644 0d93348... 9282c0c... M > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInternals.java > :100644 100644 2260a45... 02a39be... M > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java > :100644 100644 8cfe076... 8fce0dd... M > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestSubmissionHandling.java > :100644 100644 d840a78... 328f9b0... M > spi/src/main/java/org/apache/sqoop/connector/spi/MetadataUpgrader.java > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/model/FormUtils.java d9666c8 > common/src/main/java/org/apache/sqoop/model/MConnection.java e5a4fb8 > common/src/main/java/org/apache/sqoop/model/MJob.java 11839fc > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnectorMetadataUpgrader.java > 2b12009 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcValidator.java > eea86b2 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnector.java > 557091e > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsMetadataUpgrader.java > PRE-CREATION > core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java b92ff4d > core/src/main/java/org/apache/sqoop/connector/ConnectorManagerUtils.java > PRE-CREATION > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 9b64661 > core/src/main/java/org/apache/sqoop/repository/Repository.java e9c32e0 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java > 030dde7 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > 88be9fb > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java > 1a77360 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java > e5bb2e7 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java > f603cc1 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectionHandling.java > bdd3c05 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java > 54ae726 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestFrameworkHandling.java > 8b3326d > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInputTypes.java > 5d3807d > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInternals.java > 0d93348 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java > 2260a45 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestSubmissionHandling.java > 8cfe076 > spi/src/main/java/org/apache/sqoop/connector/spi/MetadataUpgrader.java > d840a78 > > Diff: https://reviews.apache.org/r/24957/diff/ > > > Testing > ------- > > Ran derby repository tests manually. > > > Thanks, > > Abraham Elmahrek > >
