Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Sqoop >> mail # dev >> Review Request: SQOOP-659. Design metadata upgrade procedure


Copy link to this message
-
Re: Review Request: SQOOP-659. Design metadata upgrade procedure

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10281/#review19168
-----------------------------------------------------------
Hi Hari,
thank you very much for working on this big JIRA. Please accept my apologies for so many review rounds. I've dive very deeply into the patch and I have several comments:
core/src/main/java/org/apache/sqoop/repository/Repository.java
<https://reviews.apache.org/r/10281/#comment39752>

    I think that this method should be final. As this functionality should be exactly the same across all repository implementations, repositories should not be given chance to override it.

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

    I'm not sure that this will actually work. The updateConnector() will try to remove forms and inputs that are referenced by connection and job which I think will lead to SQLExceptions. Maybe we should consider adding tests to confirm that.
    
    Also it seems that it doesn't make sense to use updateConnector() and udateConnectorForms() methods separately, so what about merging them together?

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

    I think that at this point we should not instantiate the Connector class, we should get the one that is already instantiated in the running server.

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

    I think that we should create here expected form structure from the Connector itself, rather than supplying empty forms, so that Upgrader will be responsible only for transferring actual values. E.g.:
    
    new MConnection(connectorId,
      newConnector.getConnectionForms(),
      FrameworkManager.getFramework().getConnectionForms()
    );

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

    I think that we should create here expected form structure from the Connector itself, rather than supplying empty forms, so that Upgrader will be responsible only for transferring actual values. E.g.:
    
    new MJob(connectorId, oldJob.getType()
      newConnector.getJobForms(oldJob.getType()),
      FrameworkManager.getFramework().getJobForms(oldJob.getType())
    );

repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java
<https://reviews.apache.org/r/10281/#comment39751>

    Nit: Would you mind using same indentation and position of + sign as other queries in the file?

repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java
<https://reviews.apache.org/r/10281/#comment39750>

    Nit: Would you mind using same indentation and position of + sign as other queries in the file?

repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java
<https://reviews.apache.org/r/10281/#comment39748>

    Nit: Would you mind either increase the intend here or put the "=?" on previous line?

repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java
<https://reviews.apache.org/r/10281/#comment39746>

    Nit: Would you mind to increase the indent here or put the "ON" clause on the same line?

spi/src/main/java/org/apache/sqoop/connector/spi/MetadataUpgrader.java
<https://reviews.apache.org/r/10281/#comment39753>

    I do have second thoughs on the Upgrader interface. We are passing entire Connection/Job object to the connector which is actually a security hole as the connector should not even see that. I would suggest to change the interface to pass only the ConnectionForms/JobForms for the connector itself.
Jarcec

- Jarek Cecho
On April 14, 2013, 9:34 a.m., Hari Shreedharan wrote:
>
> ---------------------