> On April 17, 2013, 11:09 p.m., Jarek Cecho wrote: > > Hi Hari, > > thank you very much for your effort to get this in. I think that we're > > almost there! > > Hari Shreedharan wrote: > Thanks for the review, Jarcec. I have posted an updated patch which fixes > most of the issues you pointed out.
Thank you! I'll take a look shortly. > On April 17, 2013, 11:09 p.m., Jarek Cecho wrote: > > common/src/main/java/org/apache/sqoop/model/MJob.java, lines 121-127 > > <https://reviews.apache.org/r/10281/diff/5/?file=281617#file281617line121> > > > > I do not feel comfortable with exposing this interface. Model classes > > are (will be) part of public API, so I would prefer not to expose ability > > to switch forms in place and rather create new objects during the upgrade > > procedure. > > Hari Shreedharan wrote: > If that is the case, we should remove the public constructors and move > them to a builder/factory model - that clearly shows that these are not meant > to be added/removed. Also we should probably return immutable lists in the > getters (else I can change anything I want through that). Agreed, we can cover those in follow up JIRA. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10281/#review19320 ----------------------------------------------------------- On April 18, 2013, 6:58 a.m., Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10281/ > ----------------------------------------------------------- > > (Updated April 18, 2013, 6:58 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 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcConnectorMetadataUpgrader.java > PRE-CREATION > 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 > >
