-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6250/
-----------------------------------------------------------

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

-----------------------------------------------------------
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
>
>

> 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
>
>

> 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
>
>

> 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
>
>

-----------------------------------------------------------
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.
Changes
-------

I've applied Abhijeet's suggestions.
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 (updated)
-----

  /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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6250/#review9696
-----------------------------------------------------------

Ship it!
Last two comments on the Logs, if possible please incorporate them. No need of uploading new diff, I am good with the functionality changes.
"ant test -Dhadoopversion=100" ran a success for me.

Thanks for the patch.
/src/java/org/apache/sqoop/ConnFactory.java
<https://reviews.apache.org/r/6250/#comment20668>

    Change "found" to "find" in the log message.

/src/java/org/apache/sqoop/ConnFactory.java
<https://reviews.apache.org/r/6250/#comment20669>

    change "supports" to "support" in the Log message.
- Abhijeet Gaikwad
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
>
>

> On Aug. 1, 2012, 6:37 p.m., Abhijeet Gaikwad wrote:
> > Last two comments on the Logs, if possible please incorporate them. No need of uploading new diff, I am good with the functionality changes.
> > "ant test -Dhadoopversion=100" ran a success for me.
> >
> > Thanks for the patch.

Updated version has been attached to the JIRA.
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6250/#review9696
-----------------------------------------------------------
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
>
>

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6250/#review9788
-----------------------------------------------------------

Ship it!

/src/java/org/apache/sqoop/ConnFactory.java
<https://reviews.apache.org/r/6250/#comment20778>

    One nit: add a space before the word "Sqoop".
- Bilung Lee
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
>
>

> On Aug. 2, 2012, 9:48 p.m., Bilung Lee wrote:
> > /src/java/org/apache/sqoop/ConnFactory.java, line 128
> > <https://reviews.apache.org/r/6250/diff/2/?file=132307#file132307line128>
> >
> >     One nit: add a space before the word "Sqoop".

Thanks for your review sir,
I've uploaded updated patch to JIRA, feel free to commit it at will.

Jarcec
- Jarek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6250/#review9788
-----------------------------------------------------------
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
>
>