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 Tue, 21 Oct 2014 13:31:24 GMT


> On Oct. 20, 2014, 11:47 p.m., Jarek Cecho wrote:
> > core/src/main/java/org/apache/sqoop/driver/DriverConfigurableUpgrader.java, line
26
> > <https://reviews.apache.org/r/26880/diff/9-10/?file=726340#file726340line26>
> >
> >     I'm wondering why we are changing the type from abstract class to interface
here? I would keep it as abstract class to correspond with the other upgrader even thought
that we are not going to create multiple implentations nor we care that much about backward
compatibilty here since it's internal class.
> 
> Veena Basavaraj wrote:
>     there is no way we will have more than one driver and thus a driver not having a
implementation for a method we add is absurdand also it is something we control. \
> 
> Jarek Cecho wrote:
>     I do agree with that the driver upgrader will be always only one. But I don't see
a reason why not be consistent - ConnectorUpgrader is a class, so why would we make DriverUpgrader
an interface?
> 
> Veena Basavaraj wrote:
>     There is reason somethings are interface and something are coded as classes.
>     
>     A class makes sense when we foresee default implementations. 
>     
>     I dont see that for  Driver. If at all we have multiple drivers and each could behave
differently it would compeltely make sense.
>     
>     As far as consistency agreement, it is already a moot, Driver and Connectors are
not the same, they just happen to have one thing in common that they expose configs.
>     
>     Second, Driver ugrader is not even part of the public api.
> 
> Jarek Cecho wrote:
>     Following your argument, then it don't make sense to introduce the interface in the
first place, right? If we do not need the ugpraders (=connector, driver, foobar) to "look
the same", then we don't need interface here as we're never going to have multiple implementations
of it.
>     
>     I originally did not mentioned this as I assumed that our goal is to have the same
similar interface for all upgraders, so that thay can be operated in similar way where appropriate.
If it's not the case, then I guess we can drop the interface all together and just keep the
concrete implementation of the Driver upgrader.
> 
> Veena Basavaraj wrote:
>     suggested the same yday. less code even better. There is no need of a extra interface
for the sake of it. cool

Agreed then.


- Jarek


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


On Oct. 21, 2014, 1:22 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26880/
> -----------------------------------------------------------
> 
> (Updated Oct. 21, 2014, 1:22 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/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/ConfigurableError.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