----------------------------------------------------------- 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: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10281/ > ----------------------------------------------------------- > > (Updated April 14, 2013, 9:34 a.m.) > > > Review request for Sqoop. > > > Description > ------- > > Metadata upgrade for connector upgrades. This is an initial patch soliciting > feedback. Limited testing has been done. I will add some unit tests once I > get feedback. > > > This addresses bug SQOOP-659. > https://issues.apache.org/jira/browse/SQOOP-659 > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/model/MConnection.java 36dca42 > common/src/main/java/org/apache/sqoop/model/MJob.java a53f04e > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnector.java > c315e48 > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 32df1e5 > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java > ca51313 > core/src/main/java/org/apache/sqoop/repository/Repository.java d6ec303 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java > 95f6570 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > 32cef8a > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java > ea458ac > spi/src/main/java/org/apache/sqoop/connector/spi/MetadataUpgrader.java > PRE-CREATION > spi/src/main/java/org/apache/sqoop/connector/spi/SqoopConnector.java > 540303a > > Diff: https://reviews.apache.org/r/10281/diff/ > > > Testing > ------- > > > Thanks, > > Hari Shreedharan > >
