> On Aug. 21, 2015, 9:16 p.m., Abraham Elmahrek wrote: > > It seems good to me except you're missing some of the upgrade logic. Or > > perhaps removed fields simply won't be transfered? Either way, a warning > > printed when upgrading might be a good idea. Or at least something in the > > logs.
Good point, I should have been more descriptive in my comment. The upgrade path is tricky. Our existing logic is "you need to enter either 'table' or 'sql'" and this patch drops the 'sql'. Hence during upgrade, if user had configured 'sql', the job object became invalid and thus the entire upgrade fails. Forcing user to firstly reconfigure their jobs and drop the 'sql' field value before the ugprade. I'm not anticipating this to be a huge problem because as I've mentioned on the JIRA, this functionality is currently completely broken and doesn't work. Hence I'm not expecting users to have objects with 'sql' filled up. I completely agree that this is far from ideal state, but I don't see a better option how to remove required fields with current infrastructure. I was thinking about introducing upgrade mode that would disable objects during failed upgrade, but that seems outside of scope for this JIRA. I'm open to feedback though if you have any better idea! - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37596/#review96093 ----------------------------------------------------------- On Aug. 20, 2015, 4:21 p.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37596/ > ----------------------------------------------------------- > > (Updated Aug. 20, 2015, 4:21 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2441 > https://issues.apache.org/jira/browse/SQOOP-2441 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > I've dropped all code relevant to this feature. > > The patch doesn't deal with upgrade path in case that someone is using this > feature to a world where this feature is not available. As the feature is > completely broken (e.g. doesn't work at all), I'm kind of assuming that it > shouldn't be a problem. > > > Diffs > ----- > > > common/src/main/java/org/apache/sqoop/error/code/GenericJdbcConnectorError.java > 9a9bb66 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/GenericJdbcToInitializer.java > ad375fd > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfig.java > c9651d5 > > connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestGenericJdbcConnectorUpgrader.java > c39aabc > > connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java > 870ce98 > > Diff: https://reviews.apache.org/r/37596/diff/ > > > Testing > ------- > > Unit tests are passing. > > > Thanks, > > Jarek Cecho > >
