> On Oct. 20, 2014, 8:11 p.m., Jarek Cecho wrote: > > spi/src/main/java/org/apache/sqoop/connector/spi/ConnectorConfigurableUpgrader.java, > > lines 42-66 > > <https://reviews.apache.org/r/26880/diff/7/?file=726213#file726213line42> > > > > We should throw an exception in the default case because it means that > > Sqoop is trying to upgrade something that the connector developper did not > > anticipated. I don't think that no-oop is correct behaviour here - we > > should die early. > > Veena Basavaraj wrote: > this is a empty method for reason. There will be cases in connector that > may not have the from part. see the HbaseToConnector. > > or Link config is missing in HDSF > > In that case, why would we throw an exception? Not making sense to me. > > Jarek Cecho wrote: > If the connector don't support given entity than it's upgrade method > should not be even called in my mind. And we should ensure that it's the case > rather then silently ignoring such case. > > Similarly if the connector developper "forgets" to implement upgrade for > entity that he is using, we should be very direct about that and fail as soon > as possible, rather then wait to see if for example validations will fail > (because they don't). > > Veena Basavaraj wrote: > this is not what the current design does. > > There is no way for a connector to tell that it supports only > FromJobConfig. Even if they told, we dont store such info. > > The upgrade code in Derby, blindly calls all the methods. Am I missing > something here? since you seem to allude at things I dont seen in the code. > > Veena Basavaraj wrote: > this is the code in the the abstract class Repository, it will call these > methods for every connector. > > > public final void upgradeConnector(MConnector oldConnector, MConnector > newConnector) { > LOG.info("Upgrading connector: " + oldConnector.getUniqueName()); > long connectorID = oldConnector.getPersistenceId(); > newConnector.setPersistenceId(connectorID); > > RepositoryTransaction tx = null; > try { > SqoopConnector connector = > ConnectorManager.getInstance().getSqoopConnector( > newConnector.getUniqueName()); > > Validator connectorConfigValidator = connector.getConfigValidator(); > boolean upgradeSuccessful = true; > // 1. Get an upgrader for the connector > ConnectorConfigurableUpgrader upgrader = > connector.getConfigurableUpgrader(); > // 2. Get all links associated with the connector. > List<MLink> existingLinksByConnector = > findLinksForConnector(connectorID); > // 3. Get all jobs associated with the connector. > List<MJob> existingJobsByConnector = > findJobsForConnector(connectorID); > // -- BEGIN TXN -- > tx = getTransaction(); > tx.begin(); > // 4. Delete the inputs for all of the jobs and links (in that > order) for > // this connector > deletelinksAndJobs(existingLinksByConnector, > existingJobsByConnector, tx); > // 5. Delete all inputs and configs associated with the connector, > and > // insert the new configs and inputs for this connector > upgradeConnectorConfigs(newConnector, tx); > // 6. Run upgrade logic for the configs related to the link objects > for (MLink link : existingLinksByConnector) { > // Make a new copy of the configs > List<MConfig> linkConfig = > newConnector.getLinkConfig().clone(false).getConfigs(); > MLinkConfig newLinkConfig = new MLinkConfig(linkConfig); > MLinkConfig oldLinkConfig = link.getConnectorLinkConfig(); > upgrader.upgradeLinkConfig(oldLinkConfig, newLinkConfig); > MLink newlink = new MLink(link, newLinkConfig); > > Object newConfigurationObject = ClassUtils.instantiate(connector > .getLinkConfigurationClass()); > > ConfigUtils.fromConfigs(newlink.getConnectorLinkConfig().getConfigs(), > newConfigurationObject); > // 7. Run link config validation > ConfigValidator configValidator = connectorConfigValidator > .validateConfigForLink(newConfigurationObject); > if (configValidator.getStatus().canProceed()) { > updateLink(newlink, tx); > } else {
I disagree that we are blindly calling all upgrade methods in all cases. The upgrade method for MLink is called inside of "for(Mlink)" statement. E.g. if there will be no MLink object to upgrade, then it's corresponding upgrade method will be never called. And it works the other way around, if there is an MLink object, we will always call the MLink's upgrade method. And in that case, connector developper should always provide upgrade procedure. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26880/#review57419 ----------------------------------------------------------- On Oct. 20, 2014, 8:09 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, 8:09 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/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 > >
