> On Oct. 20, 2014, 11:47 p.m., Jarek Cecho wrote: > > spi/src/main/java/org/apache/sqoop/connector/ConfigurableError.java, lines > > 25-27 > > <https://reviews.apache.org/r/26880/diff/10/?file=726601#file726601line25> > > > > Can we please keep the logic as is in the other ErrorCodes and keep the > > name as relatively short constant? Something like CONFIGURABLE_0001. > > Veena Basavaraj wrote: > I am not sure what the logic is, educate me. Again not sure what the > reasoning for a 0001 is, I will add it for consistency > > Jarek Cecho wrote: > It's a consistency point - all other ErrorCodes follows pattern where we > have very small error codes such as CONFIGURABLE_0001 and the explanation is > available in the message itself. > > Veena Basavaraj wrote: > Sure, I was just curious if it had any interesting story behing it. > > I still dont see how a configurable will ever suprass more than 10. ! > > > But that is just an observation, 4 digits or 2 is still obscure to me, I > would have preferred a much desciptive name, instead of the codes!
The number of zeros is actually not consistent across the code base, so I'm more then happy to have two or three :) > 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. 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. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26880/#review57466 ----------------------------------------------------------- On Oct. 20, 2014, 11:58 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26880/ > ----------------------------------------------------------- > > (Updated Oct. 20, 2014, 11:58 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/DriverConfigurableUpgrader.java > PRE-CREATION > 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 > >
