----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26880/#review57335 -----------------------------------------------------------
core/src/main/java/org/apache/sqoop/repository/Repository.java <https://reviews.apache.org/r/26880/#comment97994> I found the comment quite helpful to explain how is the upgrade done, is there a good reason to remove it? spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgrader.java <https://reviews.apache.org/r/26880/#comment97996> I have two concerns with this approach: * If given configType do have multiple variants (such as Job have two "sub types" - From and To), then we are forcing connector developper to make an educated guess what structures are handled to him for upgrade. * It's up to connector developer to properly throw an exception if we are upgrading configType that is not supported by the connector itself. I was thinkinking how to resolve both concers and I've came up with following idea: What about having one generic ConfigurationUpgrader (as we have now) and have one method per config type (similarly as we had before), but let's provide concrete default implementation that will throw NotImplementedException (or something similar). Then the concerns will be address as: * It's easy to add additional "sub type" information to each method for each config type * Provided default implementation will properly thrown an exception in case that Sqoop is calling something that should not be called As the ConfigurableUpgrader is abstract class we can easily add new methods to it in the future as we will add additional config types without breaking backward compatibility. What do you think? common/src/main/java/org/apache/sqoop/model/Configurable.java <https://reviews.apache.org/r/26880/#comment97993> What is the reason to rename the MConfigurable to Configurable? All our model classes are starting with "M" to denote that the are models. spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgradeUtil.java <https://reviews.apache.org/r/26880/#comment97995> This file is missing licence and test. - Jarek Cecho On Oct. 20, 2014, 2:38 a.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26880/ > ----------------------------------------------------------- > > (Updated Oct. 20, 2014, 2:38 a.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > See JIRA : https://issues.apache.org/jira/browse/SQOOP-1551 > > for the details of the ticket and the changes in the RB > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/model/Configurable.java PRE-CREATION > common/src/main/java/org/apache/sqoop/model/MConfigurableType.java > PRE-CREATION > common/src/main/java/org/apache/sqoop/model/MConnector.java 2f42191 > common/src/main/java/org/apache/sqoop/model/MDriver.java 685439e > common/src/main/java/org/apache/sqoop/model/MJob.java b3dec27 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnector.java > 87ac2af > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnectorUpgrader.java > a069b3e > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConfigUpgrader.java > b17aa21 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnector.java > 606b9fa > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnectorUpgrader.java > PRE-CREATION > core/src/main/java/org/apache/sqoop/connector/ConnectorHandler.java 54bdd13 > core/src/main/java/org/apache/sqoop/connector/ConnectorManager.java 5226926 > core/src/main/java/org/apache/sqoop/driver/Driver.java f1b45bb > core/src/main/java/org/apache/sqoop/driver/DriverConfigUpgrader.java > 847b73d > core/src/main/java/org/apache/sqoop/driver/DriverConfigValidator.java > 9c3b660 > core/src/main/java/org/apache/sqoop/driver/DriverUpgrader.java PRE-CREATION > core/src/main/java/org/apache/sqoop/driver/JobManager.java df2a5ab > > core/src/main/java/org/apache/sqoop/driver/configuration/DriverConfiguration.java > d4e2254 > > core/src/main/java/org/apache/sqoop/driver/configuration/JobConfiguration.java > PRE-CREATION > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java f06fd0c > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java > 5a8e026 > core/src/main/java/org/apache/sqoop/repository/Repository.java 74a9e12 > core/src/test/java/org/apache/sqoop/driver/TestDriverConfigUpgrader.java > dc4e8c8 > core/src/test/java/org/apache/sqoop/driver/TestJobManager.java 3b475c6 > core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java > 34bd8a5 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > c888910 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestDriverHandling.java > 95fbe07 > server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java > 462579c > server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java > 80e65b8 > > spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgradeUtil.java > PRE-CREATION > spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgrader.java > PRE-CREATION > spi/src/main/java/org/apache/sqoop/connector/spi/RepositoryUpgrader.java > 879e428 > spi/src/main/java/org/apache/sqoop/connector/spi/SqoopConnector.java > 5315e1f > tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java > 76ebd3b > > Diff: https://reviews.apache.org/r/26880/diff/ > > > Testing > ------- > > yes > > > Thanks, > > Veena Basavaraj > >
