You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@sqoop.apache.org by Veena Basavaraj <vb...@cloudera.com> on 2014/10/17 19:53:13 UTC

Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

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

Review request for Sqoop.


Repository: sqoop-sqoop2


Description
-------

#1 - + 1 from Jarek Jarcec Cecho, we shall call it ConfigurableUpgrader


Diffs
-----

  common/src/main/java/org/apache/sqoop/model/MConfigurable.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 
  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/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/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 44ec2e3 
  server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 462579c 
  server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java 80e65b8 
  shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a1bc5d5 
  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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Oct. 18, 2014, 9:27 a.m., Jarek Cecho wrote:
> > spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgrader.java, line 50
> > <https://reviews.apache.org/r/26880/diff/2/?file=724556#file724556line50>
> >
> >     Was exactly my concern :) Can we fille a JIRA for that and create put the JIRA number there directly?

The JIRA is arelady there, it is part of this JIRA, please see the comments in the JIRA, this is the point where I needed to discsus if we need ConnectorUpgrader and DriverUpgrader subclasses? rather than interfaces.


> On Oct. 18, 2014, 9:27 a.m., Jarek Cecho wrote:
> > common/src/main/java/org/apache/sqoop/model/MConnector.java, lines 20-21
> > <https://reviews.apache.org/r/26880/diff/2/?file=724531#file724531line20>
> >
> >     This import doesn't seem valid?

good catch!


- Veena


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


On Oct. 17, 2014, 11:08 a.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26880/
> -----------------------------------------------------------
> 
> (Updated Oct. 17, 2014, 11:08 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> #1 - + 1 from Jarek Jarcec Cecho, we shall call it ConfigurableUpgrader
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/model/MConfigurable.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 
>   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/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/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 44ec2e3 
>   server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 462579c 
>   server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java 80e65b8 
>   shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a1bc5d5 
>   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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Oct. 18, 2014, 9:27 a.m., Jarek Cecho wrote:
> > spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgrader.java, line 50
> > <https://reviews.apache.org/r/26880/diff/2/?file=724556#file724556line50>
> >
> >     Was exactly my concern :) Can we fille a JIRA for that and create put the JIRA number there directly?
> 
> Veena Basavaraj wrote:
>     The JIRA is arelady there, it is part of this JIRA, please see the comments in the JIRA, this is the point where I needed to discsus if we need ConnectorUpgrader and DriverUpgrader subclasses? rather than interfaces.
> 
> Jarek Cecho wrote:
>     I'm confused, if we want to revisit the API as a part of this JIRA, why the patch is adding note that it will be done in the future?
>     
>     Anyway I'm wondering what are your thoughts about separate Connector and Driver upgrader?
>     
>     The current code forces the connector developper to detect whether we gave him "From" or "To" job forms which doesn't seem right. I think that we should add a parameter Direction that will specify what direction are we upgrading.
> 
> Veena Basavaraj wrote:
>     but direction does not make sense for every config type that we will add
>     
>     it is not in the current patch, since I still dont see any comments in JIRA validating the proposal I have
>     
>     Please read my comment in JIRA, it says I have part1 and part2 for the same reason, I can wait for this discussion to proceed and then upload a full patch ( both part 1 and part 2)
>     
>     I kindly request that you think about this carefully.
>     
>     Each configurable behaves differently, for instance Connector and Driver expose configs but are very different entities, so does it makes sense to have one interface for all objects that expose configs?
>     
>     second thing that I have noticed, the configurables themselves do not tell what type of config they persist, we decide in SQOOP that it is of LINK/ JOB, this makes it really hard to have an api that is self aware for these configurables.
>     
>     Let me know if I am making any sense to you:)

Jarcec, I have updated a new patch, that aligns with having a single extensible api for upgrades. Let me know what your thoughts are. 

This resolves all the issues that was supposed to addressed by SQOOP -1551


- Veena


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


On Oct. 17, 2014, 11:08 a.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26880/
> -----------------------------------------------------------
> 
> (Updated Oct. 17, 2014, 11:08 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> #1 - + 1 from Jarek Jarcec Cecho, we shall call it ConfigurableUpgrader
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/model/MConfigurable.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 
>   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/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/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 44ec2e3 
>   server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 462579c 
>   server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java 80e65b8 
>   shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a1bc5d5 
>   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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.


> On Oct. 18, 2014, 4:27 p.m., Jarek Cecho wrote:
> > spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgrader.java, line 50
> > <https://reviews.apache.org/r/26880/diff/2/?file=724556#file724556line50>
> >
> >     Was exactly my concern :) Can we fille a JIRA for that and create put the JIRA number there directly?
> 
> Veena Basavaraj wrote:
>     The JIRA is arelady there, it is part of this JIRA, please see the comments in the JIRA, this is the point where I needed to discsus if we need ConnectorUpgrader and DriverUpgrader subclasses? rather than interfaces.

I'm confused, if we want to revisit the API as a part of this JIRA, why the patch is adding note that it will be done in the future?

Anyway I'm wondering what are your thoughts about separate Connector and Driver upgrader?

The current code forces the connector developper to detect whether we gave him "From" or "To" job forms which doesn't seem right. I think that we should add a parameter Direction that will specify what direction are we upgrading.


- Jarek


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


On Oct. 17, 2014, 6:08 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26880/
> -----------------------------------------------------------
> 
> (Updated Oct. 17, 2014, 6:08 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> #1 - + 1 from Jarek Jarcec Cecho, we shall call it ConfigurableUpgrader
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/model/MConfigurable.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 
>   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/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/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 44ec2e3 
>   server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 462579c 
>   server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java 80e65b8 
>   shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a1bc5d5 
>   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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Oct. 18, 2014, 9:27 a.m., Jarek Cecho wrote:
> > spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgrader.java, line 50
> > <https://reviews.apache.org/r/26880/diff/2/?file=724556#file724556line50>
> >
> >     Was exactly my concern :) Can we fille a JIRA for that and create put the JIRA number there directly?
> 
> Veena Basavaraj wrote:
>     The JIRA is arelady there, it is part of this JIRA, please see the comments in the JIRA, this is the point where I needed to discsus if we need ConnectorUpgrader and DriverUpgrader subclasses? rather than interfaces.
> 
> Jarek Cecho wrote:
>     I'm confused, if we want to revisit the API as a part of this JIRA, why the patch is adding note that it will be done in the future?
>     
>     Anyway I'm wondering what are your thoughts about separate Connector and Driver upgrader?
>     
>     The current code forces the connector developper to detect whether we gave him "From" or "To" job forms which doesn't seem right. I think that we should add a parameter Direction that will specify what direction are we upgrading.

but direction does not make sense for every config type that we will add

it is not in the current patch, since I still dont see any comments in JIRA validating the proposal I have

Please read my comment in JIRA, it says I have part1 and part2 for the same reason, I can wait for this discussion to proceed and then upload a full patch ( both part 1 and part 2)

I kindly request that you think about this carefully.

Each configurable behaves differently, for instance Connector and Driver expose configs but are very different entities, so does it makes sense to have one interface for all objects that expose configs?

second thing that I have noticed, the configurables themselves do not tell what type of config they persist, we decide in SQOOP that it is of LINK/ JOB, this makes it really hard to have an api that is self aware for these configurables.

Let me know if I am making any sense to you:)


- Veena


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


On Oct. 17, 2014, 11:08 a.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26880/
> -----------------------------------------------------------
> 
> (Updated Oct. 17, 2014, 11:08 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> #1 - + 1 from Jarek Jarcec Cecho, we shall call it ConfigurableUpgrader
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/model/MConfigurable.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 
>   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/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/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 44ec2e3 
>   server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 462579c 
>   server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java 80e65b8 
>   shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a1bc5d5 
>   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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.

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


Looks mostly good, I have two minor nits:


common/src/main/java/org/apache/sqoop/model/MConnector.java
<https://reviews.apache.org/r/26880/#comment97813>

    This import doesn't seem valid?



spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgrader.java
<https://reviews.apache.org/r/26880/#comment97814>

    Was exactly my concern :) Can we fille a JIRA for that and create put the JIRA number there directly?


Jarcec

- Jarek Cecho


On Oct. 17, 2014, 6:08 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26880/
> -----------------------------------------------------------
> 
> (Updated Oct. 17, 2014, 6:08 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> #1 - + 1 from Jarek Jarcec Cecho, we shall call it ConfigurableUpgrader
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/model/MConfigurable.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 
>   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/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/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 44ec2e3 
>   server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 462579c 
>   server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java 80e65b8 
>   shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a1bc5d5 
>   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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.


> On Oct. 18, 2014, 5:10 a.m., Qian Xu wrote:
> > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnectorUpgrader.java, line 72
> > <https://reviews.apache.org/r/26880/diff/2/?file=724537#file724537line72>
> >
> >     The doUpgrade code appears multiple times in code. Better create a follow-up jira to move this code into its superclass.
> 
> Veena Basavaraj wrote:
>     Thats a very good observation. I found the same with other parts of the connectors code as well, between the driver and connector a lot of the configuration classes can be shared, instead they are copy pasted, thanks for the input, I will move it to a util class

Good catch, another option is to refactore the copy&pasted parts into Util class and put it into connector SDK.


- Jarek


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


On Oct. 17, 2014, 6:08 p.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26880/
> -----------------------------------------------------------
> 
> (Updated Oct. 17, 2014, 6:08 p.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> #1 - + 1 from Jarek Jarcec Cecho, we shall call it ConfigurableUpgrader
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/model/MConfigurable.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 
>   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/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/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 44ec2e3 
>   server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 462579c 
>   server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java 80e65b8 
>   shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a1bc5d5 
>   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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Oct. 17, 2014, 10:10 p.m., Qian Xu wrote:
> > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnectorUpgrader.java, line 72
> > <https://reviews.apache.org/r/26880/diff/2/?file=724537#file724537line72>
> >
> >     The doUpgrade code appears multiple times in code. Better create a follow-up jira to move this code into its superclass.

Thats a very good observation. I found the same with other parts of the connectors code as well, between the driver and connector a lot of the configuration classes can be shared, instead they are copy pasted, thanks for the input, I will move it to a util class


- Veena


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


On Oct. 17, 2014, 11:08 a.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26880/
> -----------------------------------------------------------
> 
> (Updated Oct. 17, 2014, 11:08 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> #1 - + 1 from Jarek Jarcec Cecho, we shall call it ConfigurableUpgrader
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/model/MConfigurable.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 
>   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/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/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 44ec2e3 
>   server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 462579c 
>   server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java 80e65b8 
>   shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a1bc5d5 
>   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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Oct. 17, 2014, 10:10 p.m., Qian Xu wrote:
> > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnectorUpgrader.java, line 72
> > <https://reviews.apache.org/r/26880/diff/2/?file=724537#file724537line72>
> >
> >     The doUpgrade code appears multiple times in code. Better create a follow-up jira to move this code into its superclass.
> 
> Veena Basavaraj wrote:
>     Thats a very good observation. I found the same with other parts of the connectors code as well, between the driver and connector a lot of the configuration classes can be shared, instead they are copy pasted, thanks for the input, I will move it to a util class
> 
> Jarek Cecho wrote:
>     Good catch, another option is to refactore the copy&pasted parts into Util class and put it into connector SDK.

I cannot move it to conenctor sdk, since the same upgrade code is copy pasted in driver as well :)

So I have added the util in the spi package for configurable.


- Veena


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


On Oct. 19, 2014, 5:20 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, 5:20 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/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 44ec2e3 
>   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 
>   shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a1bc5d5 
>   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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Qian Xu <sx...@googlemail.com>.

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



connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/HdfsConnectorUpgrader.java
<https://reviews.apache.org/r/26880/#comment97810>

    The doUpgrade code appears multiple times in code. Better create a follow-up jira to move this code into its superclass.


- Qian Xu


On Oct. 18, 2014, 2:08 a.m., Veena Basavaraj wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26880/
> -----------------------------------------------------------
> 
> (Updated Oct. 18, 2014, 2:08 a.m.)
> 
> 
> Review request for Sqoop.
> 
> 
> Repository: sqoop-sqoop2
> 
> 
> Description
> -------
> 
> #1 - + 1 from Jarek Jarcec Cecho, we shall call it ConfigurableUpgrader
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/sqoop/model/MConfigurable.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 
>   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/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/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 44ec2e3 
>   server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 462579c 
>   server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java 80e65b8 
>   shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a1bc5d5 
>   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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Oct. 20, 2014, 8:36 a.m., Jarek Cecho wrote:
> > spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgradeUtil.java, line 1
> > <https://reviews.apache.org/r/26880/diff/5/?file=725505#file725505line1>
> >
> >     This file is missing licence and test.

oh I missed the test part..let me answer it !

this code was part of the exisiting connectors that I moved to a common class to avoid copy paste.

So it is not some new functionality that was added in this RB


- Veena


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


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.


> On Oct. 20, 2014, 3:36 p.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?
> 
> Veena Basavaraj wrote:
>     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.!

Your point pretty much coveres the second issue I've seen there :)


- Jarek


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


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


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

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.


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.


> On Oct. 20, 2014, 3:36 p.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?
> 
> Veena Basavaraj wrote:
>     the comment is still there, it is more contextual and explains the exact steps it actual does. see below

I like the overview on top, but sure this will work as well.


> On Oct. 20, 2014, 3:36 p.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.

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?


> On Oct. 20, 2014, 3:36 p.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.
> 
> Veena Basavaraj wrote:
>     this is not persisted, it is just a base marker class, the actual persisted classes are still M prefixed

Half of the model classes are "markers" and yet we have been keeping the "M" prefix there. Do we have another reason why we are changing that now? I would suggest to use the prefix if not.


- Jarek


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


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.

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



core/src/main/java/org/apache/sqoop/repository/Repository.java
<https://reviews.apache.org/r/26880/#comment97994>

    I found the comment quite helpful to explain how is the upgrade done, is there a good reason to remove it?



spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgrader.java
<https://reviews.apache.org/r/26880/#comment97996>

    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?



common/src/main/java/org/apache/sqoop/model/Configurable.java
<https://reviews.apache.org/r/26880/#comment97993>

    What is the reason to rename the MConfigurable to Configurable? All our model classes are starting with "M" to denote that the are models.



spi/src/main/java/org/apache/sqoop/connector/spi/ConfigurableUpgradeUtil.java
<https://reviews.apache.org/r/26880/#comment97995>

    This file is missing licence and test.


- Jarek Cecho


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


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

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


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


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

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 {


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.


> 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.
> 
> 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.
> 
> Veena Basavaraj wrote:
>     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 :)

I would use UnsupportedOperationException (or SqoopException) with something like:

"$ENTITY upgrade called, but no upgrade routine provided by implementation."

I don't have strong opinion about the sentence, so feel free to tweak it!


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.


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

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


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


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

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.


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


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

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.


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


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

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


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Oct. 20, 2014, 1:11 p.m., Jarek Cecho wrote:
> > I think that we are almost there :)
> > 
> > 1) Please ensure that your latest patch don't have trailing whitespace issues.

done.!


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.


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

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.


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.

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


I think that we are almost there :)

1) Please ensure that your latest patch don't have trailing whitespace issues.


spi/src/main/java/org/apache/sqoop/connector/spi/ConnectorConfigurableUpgrader.java
<https://reviews.apache.org/r/26880/#comment98120>

    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.


Jarcec

- Jarek Cecho


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Oct. 20, 2014, 4: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.

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


> On Oct. 20, 2014, 4:47 p.m., Jarek Cecho wrote:
> > spi/src/main/java/org/apache/sqoop/connector/ConfigurableError.java, lines 25-27
> > <https://reviews.apache.org/r/26880/diff/10/?file=726601#file726601line25>
> >
> >     Can we please keep the logic as is in the other ErrorCodes and keep the name as relatively short constant? Something like CONFIGURABLE_0001.

I am not sure what the logic is, educate me. Again not sure what the reasoning for a 0001 is, I will add it for consistency


- Veena


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


On Oct. 20, 2014, 4:49 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, 4:49 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/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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.


> On Oct. 20, 2014, 11:47 p.m., Jarek Cecho wrote:
> > spi/src/main/java/org/apache/sqoop/connector/ConfigurableError.java, lines 25-27
> > <https://reviews.apache.org/r/26880/diff/10/?file=726601#file726601line25>
> >
> >     Can we please keep the logic as is in the other ErrorCodes and keep the name as relatively short constant? Something like CONFIGURABLE_0001.
> 
> Veena Basavaraj wrote:
>     I am not sure what the logic is, educate me. Again not sure what the reasoning for a 0001 is, I will add it for consistency
> 
> Jarek Cecho wrote:
>     It's a consistency point - all other ErrorCodes follows pattern where we have very small error codes such as CONFIGURABLE_0001 and the explanation is available in the message itself.
> 
> Veena Basavaraj wrote:
>     Sure, I was just curious if it had any interesting story behing it.
>     
>     I still dont see how a configurable will ever suprass more than 10. ! 
>     
>     
>     But that is just an observation, 4 digits or 2 is still obscure to me, I would have preferred a much desciptive name, instead of the codes!

The number of zeros is actually not consistent across the code base, so I'm more then happy to have two or three :)


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

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.


- Jarek


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


On Oct. 20, 2014, 11:58 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, 11:58 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/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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Oct. 20, 2014, 4:47 p.m., Jarek Cecho wrote:
> > spi/src/main/java/org/apache/sqoop/connector/ConfigurableError.java, lines 25-27
> > <https://reviews.apache.org/r/26880/diff/10/?file=726601#file726601line25>
> >
> >     Can we please keep the logic as is in the other ErrorCodes and keep the name as relatively short constant? Something like CONFIGURABLE_0001.
> 
> Veena Basavaraj wrote:
>     I am not sure what the logic is, educate me. Again not sure what the reasoning for a 0001 is, I will add it for consistency
> 
> Jarek Cecho wrote:
>     It's a consistency point - all other ErrorCodes follows pattern where we have very small error codes such as CONFIGURABLE_0001 and the explanation is available in the message itself.

Sure, I was just curious if it had any interesting story behing it.

I still dont see how a configurable will ever suprass more than 10. ! 


But that is just an observation, 4 digits or 2 is still obscure to me, I would have preferred a much desciptive name, instead of the codes!


> On Oct. 20, 2014, 4: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?

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.


- Veena


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


On Oct. 20, 2014, 4:58 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, 4:58 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/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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.


> On Oct. 20, 2014, 4: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.

suggested the same yday. less code even better. There is no need of a extra interface for the sake of it. cool


- Veena


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


On Oct. 20, 2014, 4:58 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, 4:58 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/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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.


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

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?


> On Oct. 20, 2014, 11:47 p.m., Jarek Cecho wrote:
> > spi/src/main/java/org/apache/sqoop/connector/ConfigurableError.java, lines 25-27
> > <https://reviews.apache.org/r/26880/diff/10/?file=726601#file726601line25>
> >
> >     Can we please keep the logic as is in the other ErrorCodes and keep the name as relatively short constant? Something like CONFIGURABLE_0001.
> 
> Veena Basavaraj wrote:
>     I am not sure what the logic is, educate me. Again not sure what the reasoning for a 0001 is, I will add it for consistency

It's a consistency point - all other ErrorCodes follows pattern where we have very small error codes such as CONFIGURABLE_0001 and the explanation is available in the message itself.


- Jarek


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


On Oct. 20, 2014, 11:58 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, 11:58 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/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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.

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



core/src/main/java/org/apache/sqoop/driver/DriverConfigurableUpgrader.java
<https://reviews.apache.org/r/26880/#comment98168>

    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.



spi/src/main/java/org/apache/sqoop/connector/spi/ConnectorConfigurableUpgrader.java
<https://reviews.apache.org/r/26880/#comment98170>

    Nit: Trailing white space



spi/src/main/java/org/apache/sqoop/connector/ConfigurableError.java
<https://reviews.apache.org/r/26880/#comment98169>

    Can we please keep the logic as is in the other ErrorCodes and keep the name as relatively short constant? Something like CONFIGURABLE_0001.


Jarcec

- Jarek Cecho


On Oct. 20, 2014, 11:16 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, 11:16 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/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
> 
>

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Jarek Cecho <ja...@apache.org>.

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

Ship it!


Ship It!

- Jarek Cecho


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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.

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

(Updated Oct. 21, 2014, 6:22 a.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 (updated)
-----

  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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.

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

(Updated Oct. 20, 2014, 4:58 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 (updated)
-----

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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.

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

(Updated Oct. 20, 2014, 4:49 p.m.)


Review request for Sqoop.


Changes
-------

remove WS


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 (updated)
-----

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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.

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

(Updated Oct. 20, 2014, 4:16 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 (updated)
-----

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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.

-----------------------------------------------------------
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 (updated)
-----

  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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.

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

(Updated Oct. 20, 2014, 1:02 p.m.)


Review request for Sqoop.


Changes
-------

remove WS*!


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 (updated)
-----

  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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.

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

(Updated Oct. 20, 2014, 12:56 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 (updated)
-----

  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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.

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

(Updated Oct. 20, 2014, 11:41 a.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 (updated)
-----

  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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.

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


Changes
-------

ws* removed.


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 (updated)
-----

  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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.

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

(Updated Oct. 19, 2014, 5:20 p.m.)


Review request for Sqoop.


Repository: sqoop-sqoop2


Description (updated)
-------

See JIRA : https://issues.apache.org/jira/browse/SQOOP-1551

for the details of the ticket and the changes in the RB


Diffs (updated)
-----

  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/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 44ec2e3 
  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 
  shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a1bc5d5 
  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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.

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

(Updated Oct. 19, 2014, 5:13 p.m.)


Review request for Sqoop.


Repository: sqoop-sqoop2


Description
-------

#1 - + 1 from Jarek Jarcec Cecho, we shall call it ConfigurableUpgrader


Diffs (updated)
-----

  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/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 44ec2e3 
  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 
  shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a1bc5d5 
  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

Re: Review Request 26880: SQOOP-1551:Repository Upgrader api - Extensibility

Posted by Veena Basavaraj <vb...@cloudera.com>.

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

(Updated Oct. 17, 2014, 11:08 a.m.)


Review request for Sqoop.


Changes
-------

remove unrelated change to schema 


Repository: sqoop-sqoop2


Description
-------

#1 - + 1 from Jarek Jarcec Cecho, we shall call it ConfigurableUpgrader


Diffs (updated)
-----

  common/src/main/java/org/apache/sqoop/model/MConfigurable.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 
  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/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/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java 44ec2e3 
  server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java 462579c 
  server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java 80e65b8 
  shell/src/main/java/org/apache/sqoop/shell/core/Constants.java a1bc5d5 
  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