You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Abhijeet Gaikwad <ab...@gmail.com> on 2012/08/01 06:11:58 UTC
Re: Review Request: SQOOP-529: Enforce usage of --driver and
--connection-manager parameters
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6250/#review9674
-----------------------------------------------------------
/src/java/org/apache/sqoop/ConnFactory.java
<https://reviews.apache.org/r/6250/#comment20641>
"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.
/src/java/org/apache/sqoop/ConnFactory.java
<https://reviews.apache.org/r/6250/#comment20625>
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.
/src/java/org/apache/sqoop/ConnFactory.java
<https://reviews.apache.org/r/6250/#comment20626>
Please use "managerClassName" here which was extracted earlier in the code.
/src/java/org/apache/sqoop/ConnFactory.java
<https://reviews.apache.org/r/6250/#comment20629>
Grammatical change:
"Sqoop could not find specified Connection Manger class..."
/src/java/org/apache/sqoop/ConnFactory.java
<https://reviews.apache.org/r/6250/#comment20630>
Grammatical change:
please replace "are supporting" with "support".
/src/java/org/apache/sqoop/manager/DefaultManagerFactory.java
<https://reviews.apache.org/r/6250/#comment20638>
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.
- Abhijeet Gaikwad
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
>
>
Re: Review Request: SQOOP-529: Enforce usage of --driver and
--connection-manager parameters
Posted by Abhijeet Gaikwad <ab...@gmail.com>.
> 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.
>
> Jarek Cecho wrote:
> 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.
Pardon me this is my bad. Just getting used to Review board. I thought this was added, but again my sincere apologies, that was silly of me.
- Abhijeet
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6250/#review9674
-----------------------------------------------------------
On Aug. 1, 2012, 4:03 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6250/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2012, 4:03 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
>
>
Re: Review Request: SQOOP-529: Enforce usage of --driver and
--connection-manager parameters
Posted by Jarek Cecho <ja...@apache.org>.
> 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.
>
> Jarek Cecho wrote:
> 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.
>
> Abhijeet Gaikwad wrote:
> Pardon me this is my bad. Just getting used to Review board. I thought this was added, but again my sincere apologies, that was silly of me.
Och I see. Don't worry about it sir :-)
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6250/#review9674
-----------------------------------------------------------
On Aug. 1, 2012, 4:03 p.m., Jarek Cecho wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6250/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2012, 4:03 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
>
>
Re: Review Request: SQOOP-529: Enforce usage of --driver and
--connection-manager parameters
Posted by Jarek Cecho <ja...@apache.org>.
> 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
>
>