> On Oct. 20, 2014, 1: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 { > > Jarek Cecho wrote: > 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. > > Veena Basavaraj wrote: > I am referring to the API methods of the upgrader, I am not referring to > internals of it. > > the 3 main apis methods are > > upgradeLinkconfig > upgradeFromJobConfig > upgradeToJobConfig > > these are called on all connectors. > > Veena Basavaraj wrote: > May be I can be more articulate here. You are right in saying that we > dont call this method blindly. upgradeLinkconfig is called only if there are > link objects, so this means that it will be called on connectors who have > link configs since link objects cannot be create without the link configs. If > we are to be defensive, this should be the place where the rest/command line > api should not be allowing creating a link obkect without a link config and > throw an exception there. > > Lets look at a case that some how we have a link, without link config. In > that case do we really want an upgrade? I guess no, so whatever the connector > does at that point is moot. > > Throwing a exception in a abstract class to enforce that this method is > not called in the wrong context seems to me unnecessary when the code calling > it is already defensive about it. > > Thats my 2 cents. > > Jarek Cecho wrote: > I agree that other portions of the code are defensive enough and that > this shouldn't be a problem in normal modus operandi. In my mind bugs can > still happen though and here is super easy way to ensure that those methods > won't be executed in incorret context, so why not to do that? We have nothing > to loose and only to get.
if we are letting weird things to happen in repository that it can get to such state, then there are bigger problems to address:) having said that, please paste the exception message you'd like to see. I will add you exact comments on why this exists so we can rejoice if this exception is ever thrown in the customer logs :) - Veena ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26880/#review57419 ----------------------------------------------------------- On Oct. 20, 2014, 1: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, 1: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 > >
