sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility
Date Mon, 20 Oct 2014 21:46:25 GMT


> 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 {
> 
> 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.

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.


- 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message