sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Veena Basavaraj" <vbasava...@cloudera.com>
Subject Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility
Date Mon, 20 Oct 2014 17:58:13 GMT


> On Oct. 20, 2014, 8:36 a.m., Jarek Cecho wrote:
> > spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgrader.java, lines
32-56
> > <https://reviews.apache.org/r/26880/diff/2-5/?file=724556#file724556line32>
> >
> >     I have two concerns with this approach:
> >     
> >     * If given configType do have multiple variants (such as Job have two "sub types"
- From and To), then we are forcing connector developper to make an educated guess what structures
are handled to him for upgrade.
> >     * It's up to connector developer to properly throw an exception if we are upgrading
configType that is not supported by the connector itself.
> >     
> >     I was thinkinking how to resolve both concers and I've came up with following
idea:
> >     
> >     What about having one generic ConfigurationUpgrader (as we have now) and have
one method per config type (similarly as we had before), but let's provide concrete default
implementation that will throw NotImplementedException (or something similar). Then the concerns
will be address as:
> >     
> >     * It's easy to add additional "sub type" information to each method for each
config type
> >     * Provided default implementation will properly thrown an exception in case
that Sqoop is calling something that should not be called
> >     
> >     As the ConfigurableUpgrader is abstract class we can easily add new methods
to it in the future as we will add additional config types without breaking backward compatibility.
What do you think?
> 
> Veena Basavaraj wrote:
>     The config types are not relevant to all configurables, so I am not sure I agree
having one interface is a good idea. It leads to more of the same we have now, where things
are not applicable.
>     
>     Second, the direction is not even applicable to LINK, so whats the point of having
FROM/ To.
>     
>     Third, there is no FROM JOB type and TO JOB type
>     
>     Fourth, the upgrade logic today seems like the same for all types. so I am not sure
what is the point.
>     
>     Currently the onus is on the connector/ driver to decide how they handle each type
and that is more flexible.
> 
> Veena Basavaraj wrote:
>     Let me also add a few more things, someone keep bringing up adding "Direction" to
the upgrade api but direction is not applicable to all configurable entities.
>     
>     For Driver, there is JOB type, but it has no direction. So having direction as part
of the config type is not even applicable to all configurables.
> 
> Jarek Cecho wrote:
>     I see, that is good point that different configurables might have different "sub
types" and/or other requirements, so I think that having multiple upgraders (each per configurable
type) make sense. I would still prefer that each upgrader will follow the logic mentioned
above. E.g. that each config type will have it's own independent method that will throw exception
in case that connector developer don't care about that particular config type and that will
also enable us to pass down the "sub type" if/when needed. Do you see a trouble with that?

sure, happy to make these abstract classes and methods, so it is not even necessary to implement
it if there is no LinkConfig for a connecotor.!


- Veena


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26880/#review57335
-----------------------------------------------------------


On Oct. 19, 2014, 7:38 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26880/
> -----------------------------------------------------------
> 
> (Updated Oct. 19, 2014, 7:38 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/JobRequestHandler.java 462579c 
>   server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java 80e65b8 
>   spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgradeUtil.java PRE-CREATION

>   spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgrader.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/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