Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Sqoop uses the ConnFactory class to instantiate a ConnManager implementation based on the connect string and other arguments supplied by the user. This allows per-database logic to be encapsulated in different ConnManager instances, and dynamically chosen based on which database the user is actually importing from. But adding new ConnManager implementations requires modifying the source of a common ConnFactory class. An indirection layer should be used to delegate instantiation to a number of factory implementations which can be specified in the static configuration or at runtime.

      1. MAPREDUCE-750.patch
        17 kB
        Aaron Kimball
      2. MAPREDUCE-750.2.patch
        18 kB
        Aaron Kimball
      3. MAPREDUCE-750.3.patch
        18 kB
        Aaron Kimball
      4. MAPREDUCE-750.4.patch
        19 kB
        Aaron Kimball

        Activity

        Hide
        Aaron Kimball added a comment -

        This patch creates this public API. The sqoop.conn.factories parameter has been added to mapred-default.xml. This parameter defaults to the DefaultManagerFactory implementation supplied in Sqoop, which contains much of the logic previously in ConnManager.

        The names of additional classes implementing ManagerFactory may be added to the config parameter. The ConnFactory implementation will instantiate all such ManagerFactory implementations. When ConnFactory.getManager() is called, it will consult the accept() all ManagerFactory implementations in order, returning the first non-null value returned by an accept() method (similar to the delegation mechanism used by the Serializations framework).

        Show
        Aaron Kimball added a comment - This patch creates this public API. The sqoop.conn.factories parameter has been added to mapred-default.xml. This parameter defaults to the DefaultManagerFactory implementation supplied in Sqoop, which contains much of the logic previously in ConnManager . The names of additional classes implementing ManagerFactory may be added to the config parameter. The ConnFactory implementation will instantiate all such ManagerFactory implementations. When ConnFactory.getManager() is called, it will consult the accept() all ManagerFactory implementations in order, returning the first non-null value returned by an accept() method (similar to the delegation mechanism used by the Serializations framework).
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12413182/MAPREDUCE-750.patch
        against trunk revision 793457.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 5 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        -1 release audit. The applied patch generated 316 release audit warnings (more than the trunk's current 315 warnings).

        +1 core tests. The patch passed core unit tests.

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/383/testReport/
        Release audit warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/383/artifact/trunk/current/releaseAuditDiffWarnings.txt
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/383/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/383/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/383/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12413182/MAPREDUCE-750.patch against trunk revision 793457. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. -1 release audit. The applied patch generated 316 release audit warnings (more than the trunk's current 315 warnings). +1 core tests. The patch passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/383/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/383/artifact/trunk/current/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/383/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/383/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/383/console This message is automatically generated.
        Hide
        Aaron Kimball added a comment -

        The unit test failures are due to streaming, not sqoop. I can't figure out why there's an additional release audit warning. I've tested the patch on my system and Sqoop generates the same number of release audit warnings as it always did (4, for the .q files in testdata/hive/scripts). Hudson's link to its RAT output is dead, so I can't check the difference there. Any help?

        Show
        Aaron Kimball added a comment - The unit test failures are due to streaming, not sqoop. I can't figure out why there's an additional release audit warning. I've tested the patch on my system and Sqoop generates the same number of release audit warnings as it always did (4, for the .q files in testdata/hive/scripts). Hudson's link to its RAT output is dead, so I can't check the difference there. Any help?
        Hide
        Aaron Kimball added a comment -

        Found the proper link (http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/383/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt). The audit warning is in mapred-default.xml which I changed to add sqoop.conn.factories. If this file hasn't already had a license stamp, then this is not the time to add it...

        Show
        Aaron Kimball added a comment - Found the proper link ( http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/383/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt ). The audit warning is in mapred-default.xml which I changed to add sqoop.conn.factories . If this file hasn't already had a license stamp, then this is not the time to add it...
        Hide
        Aaron Kimball added a comment -

        I should point out – I'm all for not cluttering up the mapred-, hdfs- and core-* config files with extraneous parameters. Is there a better place to put this? Can/should we have a contrib-default and contrib-site? (Or, inevitably, per-contrib-module config files?)

        Show
        Aaron Kimball added a comment - I should point out – I'm all for not cluttering up the mapred-, hdfs- and core-* config files with extraneous parameters. Is there a better place to put this? Can/should we have a contrib-default and contrib-site? (Or, inevitably, per-contrib-module config files?)
        Hide
        Chris Douglas added a comment -

        I should point out - I'm all for not cluttering up the mapred-, hdfs- and core-* config files with extraneous parameters. Is there a better place to put this? Can/should we have a contrib-default and contrib-site? (Or, inevitably, per-contrib-module config files?)

        There are plenty of counterexamples- the contrib schedulers have taken plenty of liberties, here- but contrib modules shouldn't add to the core config. If required, please add a per-module conf dir (e.g. hdfsproxy).

        Show
        Chris Douglas added a comment - I should point out - I'm all for not cluttering up the mapred-, hdfs- and core-* config files with extraneous parameters. Is there a better place to put this? Can/should we have a contrib-default and contrib-site? (Or, inevitably, per-contrib-module config files?) There are plenty of counterexamples- the contrib schedulers have taken plenty of liberties, here- but contrib modules shouldn't add to the core config. If required, please add a per-module conf dir (e.g. hdfsproxy).
        Hide
        Aaron Kimball added a comment -

        Attaching new patch which moves the ConnFactory list parameter into sqoop-default/sqoop-site.xml. Adding this file will likely add a release warning.

        Show
        Aaron Kimball added a comment - Attaching new patch which moves the ConnFactory list parameter into sqoop-default/sqoop-site.xml. Adding this file will likely add a release warning.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12415690/MAPREDUCE-750.2.patch
        against trunk revision 801517.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 5 new or modified tests.

        -1 patch. The patch command could not apply the patch.

        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/451/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12415690/MAPREDUCE-750.2.patch against trunk revision 801517. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/451/console This message is automatically generated.
        Hide
        Aaron Kimball added a comment -

        New patch resync'd with trunk

        Show
        Aaron Kimball added a comment - New patch resync'd with trunk
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12415903/MAPREDUCE-750.3.patch
        against trunk revision 801959.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 5 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        -1 release audit. The applied patch generated 204 release audit warnings (more than the trunk's current 203 warnings).

        +1 core tests. The patch passed core unit tests.

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/459/testReport/
        Release audit warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/459/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/459/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/459/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/459/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12415903/MAPREDUCE-750.3.patch against trunk revision 801959. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. -1 release audit. The applied patch generated 204 release audit warnings (more than the trunk's current 203 warnings). +1 core tests. The patch passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/459/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/459/artifact/trunk/patchprocess/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/459/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/459/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/459/console This message is automatically generated.
        Hide
        Aaron Kimball added a comment -

        Failures are in streaming. This adds a release audit warning, as I mentioned it would.

        Show
        Aaron Kimball added a comment - Failures are in streaming. This adds a release audit warning, as I mentioned it would.
        Hide
        Tom White added a comment -
        • Rename "sqoop.conn.factories" to "sqoop.connection.factories"?
        • Rather than splitting the configuration string yourself, you could use Configuration#getStrings().
        • You should be able to avoid the release audit warning by adding the license header after the XML declaration.

        Otherwise looks good.

        Show
        Tom White added a comment - Rename "sqoop.conn.factories" to "sqoop.connection.factories"? Rather than splitting the configuration string yourself, you could use Configuration#getStrings(). You should be able to avoid the release audit warning by adding the license header after the XML declaration. Otherwise looks good.
        Hide
        Aaron Kimball added a comment -

        New patch incorporating Tom's suggestions.

        Show
        Aaron Kimball added a comment - New patch incorporating Tom's suggestions.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12417287/MAPREDUCE-750.4.patch
        against trunk revision 806577.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 5 new or modified tests.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/502/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/502/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/502/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/502/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12417287/MAPREDUCE-750.4.patch against trunk revision 806577. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 5 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/502/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/502/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/502/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-vesta.apache.org/502/console This message is automatically generated.
        Hide
        Aaron Kimball added a comment -

        Failing tests are unrelated.

        Show
        Aaron Kimball added a comment - Failing tests are unrelated.
        Hide
        Tom White added a comment -

        +1

        I've just committed this. Thanks Aaron!

        Show
        Tom White added a comment - +1 I've just committed this. Thanks Aaron!

          People

          • Assignee:
            Aaron Kimball
            Reporter:
            Aaron Kimball
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development