> 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. > > Jarek Cecho wrote: > 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? > > Veena Basavaraj wrote: > sure, happy to make these abstract classes and methods, so it is not even > necessary to implement it if there is no LinkConfig for a connecotor.!
Your point pretty much coveres the second issue I've seen there :) - 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 > >
