> On Oct. 20, 2014, 11:47 p.m., Jarek Cecho wrote: > > core/src/main/java/org/apache/sqoop/driver/DriverConfigurableUpgrader.java, > > line 26 > > <https://reviews.apache.org/r/26880/diff/9-10/?file=726340#file726340line26> > > > > I'm wondering why we are changing the type from abstract class to > > interface here? I would keep it as abstract class to correspond with the > > other upgrader even thought that we are not going to create multiple > > implentations nor we care that much about backward compatibilty here since > > it's internal class. > > Veena Basavaraj wrote: > there is no way we will have more than one driver and thus a driver not > having a implementation for a method we add is absurdand also it is something > we control. \ > > Jarek Cecho wrote: > I do agree with that the driver upgrader will be always only one. But I > don't see a reason why not be consistent - ConnectorUpgrader is a class, so > why would we make DriverUpgrader an interface? > > Veena Basavaraj wrote: > There is reason somethings are interface and something are coded as > classes. > > A class makes sense when we foresee default implementations. > > I dont see that for Driver. If at all we have multiple drivers and each > could behave differently it would compeltely make sense. > > As far as consistency agreement, it is already a moot, Driver and > Connectors are not the same, they just happen to have one thing in common > that they expose configs. > > Second, Driver ugrader is not even part of the public api. > > Jarek Cecho wrote: > Following your argument, then it don't make sense to introduce the > interface in the first place, right? If we do not need the ugpraders > (=connector, driver, foobar) to "look the same", then we don't need interface > here as we're never going to have multiple implementations of it. > > I originally did not mentioned this as I assumed that our goal is to have > the same similar interface for all upgraders, so that thay can be operated in > similar way where appropriate. If it's not the case, then I guess we can drop > the interface all together and just keep the concrete implementation of the > Driver upgrader. > > Veena Basavaraj wrote: > suggested the same yday. less code even better. There is no need of a > extra interface for the sake of it. cool
Agreed then. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26880/#review57466 ----------------------------------------------------------- On Oct. 21, 2014, 1:22 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26880/ > ----------------------------------------------------------- > > (Updated Oct. 21, 2014, 1:22 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/ConnectorRequestHandler.java > 7109ae5 > 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/configurable/ConfigurableUpgradeUtil.java > PRE-CREATION > spi/src/main/java/org/apache/sqoop/connector/ConfigurableError.java > PRE-CREATION > > spi/src/main/java/org/apache/sqoop/connector/spi/ConnectorConfigurableUpgrader.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/RepositoryDumpTool.java > f89c546 > 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 > >
