> On Aug. 1, 2012, 4:11 a.m., Abhijeet Gaikwad wrote: > > /src/java/org/apache/sqoop/ConnFactory.java, line 116 > > <https://reviews.apache.org/r/6250/diff/1/?file=131539#file131539line116> > > > > "if" constructs are not defined consistently: > > At all places I see > > if (var <cond_op> constant) > > except at one place where it is: > > if (constant <cond_op> var). > > > > I personally suggest using second type of conditional check. > > This comment is pretty minor though.
You're comment makes complete sense to me. Having different format inside one method is very strange. I'll fix one instance of the constant op var). > On Aug. 1, 2012, 4:11 a.m., Abhijeet Gaikwad wrote: > > /src/java/org/apache/sqoop/ConnFactory.java, line 125 > > <https://reviews.apache.org/r/6250/diff/1/?file=131539#file131539line125> > > > > If manager for a particular driver already exists, can we return that > > first here? > > E.g. - if driver name is - "com.mysql.jdbc.Driver", return MySqlManager > > that we already have implemented. > > > > If no matching scenario, then we can return GenericJdbcManager. As I written to the comment - this is purely for backward compatibility. I do know users that are using this "hack" in production and I definitely do not want to break their environment. > On Aug. 1, 2012, 4:11 a.m., Abhijeet Gaikwad wrote: > > /src/java/org/apache/sqoop/manager/DefaultManagerFactory.java, line 42 > > <https://reviews.apache.org/r/6250/diff/1/?file=131540#file131540line42> > > > > I think this whole change is not required here. The control flows here > > only if manualDriver and mangerClassName are NULL. Let me know if I am > > missing anything. I'm little bit confused here. I've removed that piece of code because it won't do anything as the managerClassName is being NULL. Maybe let me provide you some background for this change for better understanding. I've moved --driver and --connection-manager parameter handling out of DefaultManagerFactory to ConnFactory class. That's why this code is removed from DefaultManagerFactory. My rationale here is that user can configure additional factories and those factories than do have ability to suppress handling of --driver and --connection-manager parameters. And I do not believe that it's valid behavior. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6250/#review9674 ----------------------------------------------------------- On July 31, 2012, 8:52 p.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6250/ > ----------------------------------------------------------- > > (Updated July 31, 2012, 8:52 p.m.) > > > Review request for Sqoop. > > > Description > ------- > > I've refactored the code in the way I've designed in the JIRA. > > > This addresses bug SQOOP-529. > https://issues.apache.org/jira/browse/SQOOP-529 > > > Diffs > ----- > > /src/java/org/apache/sqoop/ConnFactory.java 1367605 > /src/java/org/apache/sqoop/manager/DefaultManagerFactory.java 1367605 > > Diff: https://reviews.apache.org/r/6250/diff/ > > > Testing > ------- > > ant tests passes > > > Thanks, > > Jarek Cecho > >
