> On Oct. 20, 2014, 3:36 p.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? > > Veena Basavaraj wrote: > the comment is still there, it is more contextual and explains the exact > steps it actual does. see below
I like the overview on top, but sure this will work as well. > On Oct. 20, 2014, 3:36 p.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? > > Veena Basavaraj wrote: > 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 Basavaraj wrote: > Let me also add a few more things, someone keep bringing up adding > "Direction" to the upgrade api but direction is not applicable to all > configurable entities. > > For Driver, there is JOB type, but it has no direction. So having > direction as part of the config type is not even applicable to all > configurables. I see, that is good point that different configurables might have different "sub types" and/or other requirements, so I think that having multiple upgraders (each per configurable type) make sense. I would still prefer that each upgrader will follow the logic mentioned above. E.g. that each config type will have it's own independent method that will throw exception in case that connector developer don't care about that particular config type and that will also enable us to pass down the "sub type" if/when needed. Do you see a trouble with that? > On Oct. 20, 2014, 3:36 p.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. > > Veena Basavaraj wrote: > this is not persisted, it is just a base marker class, the actual > persisted classes are still M prefixed Half of the model classes are "markers" and yet we have been keeping the "M" prefix there. Do we have another reason why we are changing that now? I would suggest to use the prefix if not. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26880/#review57335 ----------------------------------------------------------- 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 > >
