> On Oct. 20, 2014, 8:36 a.m., Jarek Cecho wrote: > > core/src/main/java/org/apache/sqoop/repository/Repository.java, lines > > 392-406 > > <https://reviews.apache.org/r/26880/diff/2-5/?file=724547#file724547line392> > > > > I found the comment quite helpful to explain how is the upgrade done, > > is there a good reason to remove it?
the comment is still there, it is more contextual and explains the exact steps it actual does. see below > On Oct. 20, 2014, 8:36 a.m., Jarek Cecho wrote: > > common/src/main/java/org/apache/sqoop/model/Configurable.java, line 23 > > <https://reviews.apache.org/r/26880/diff/5/?file=725476#file725476line23> > > > > What is the reason to rename the MConfigurable to Configurable? All our > > model classes are starting with "M" to denote that the are models. this is not persisted, it is just a base marker class, the actual persisted classes are still M prefixed > On Oct. 20, 2014, 8:36 a.m., Jarek Cecho wrote: > > spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgrader.java, > > lines 32-56 > > <https://reviews.apache.org/r/26880/diff/2-5/?file=724556#file724556line32> > > > > 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? The config types are not relevant to all configurables, so I am not sure I agree having one interface is a good idea. It leads to more of the same we have now, where things are not applicable. Second, the direction is not even applicable to LINK, so whats the point of having FROM/ To. Third, there is no FROM JOB type and TO JOB type Fourth, the upgrade logic today seems like the same for all types. so I am not sure what is the point. Currently the onus is on the connector/ driver to decide how they handle each type and that is more flexible. - Veena ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26880/#review57335 ----------------------------------------------------------- On Oct. 19, 2014, 7:38 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26880/ > ----------------------------------------------------------- > > (Updated Oct. 19, 2014, 7:38 p.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 > >
