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 16:33:05 GMT


> On Oct. 20, 2014, 8:36 a.m., Jarek Cecho wrote:
> > core/src/main/java/org/apache/sqoop/repository/Repository.java, lines 392-406
> > <https://reviews.apache.org/r/26880/diff/2-5/?file=724547#file724547line392>
> >
> >     I found the comment quite helpful to explain how is the upgrade done, is there
a good reason to remove it?

the comment is still there, it is more contextual and explains the exact steps it actual does.
see below


> On Oct. 20, 2014, 8:36 a.m., Jarek Cecho wrote:
> > common/src/main/java/org/apache/sqoop/model/Configurable.java, line 23
> > <https://reviews.apache.org/r/26880/diff/5/?file=725476#file725476line23>
> >
> >     What is the reason to rename the MConfigurable to Configurable? All our model
classes are starting with "M" to denote that the are models.

this is not persisted, it is just a base marker class, the actual persisted classes are still
M prefixed


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

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


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