Sqoop
  1. Sqoop
  2. SQOOP-339

Use of non-portable mknod utility causes build problems on Mac OS X

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.3.0
    • Fix Version/s: 1.4.0-incubating
    • Component/s: None
    • Labels:
      None

      Description

      Duplicating this issue from SQOOP-175, which is in the cloudera JIRA

      From SQOOP-175's description by Ken Krugler :

      On Mac OS X 10.6.3, the TestNamedFifo.testNamedFifo() test fails. The error is caused by the Shell.execCommand() use of mknod, since (at least on my Mac) mknod doesn't know about the --mode=xxx parameter or the -p parameters.

      Based on http://stackoverflow.com/questions/4546356/is-mknod-portable-if-not-what-is-the-alternative, I switched the code to use mkfifo and it now passes all tests.

      The change to NamedFifo.create() is trivial:

      Shell.execCommand("mkfifo", "-m", "0" + modeStr, filename);

      But I have no way of trying this out on other OSes, so it might create a different compatibility issue.

      1. SQOOP-339-1.patch
        1.0 kB
        Joey Echeverria
      2. SQOOP-339-2.patch
        1 kB
        Joey Echeverria
      3. SQOOP-339-3.patch
        2 kB
        Joey Echeverria
      4. SQOOP-339-4.patch
        2 kB
        Joey Echeverria
      5. SQOOP-339-5.patch
        2 kB
        Joey Echeverria
      6. SQOOP-339-6.patch
        2 kB
        Joey Echeverria

        Activity

        Hide
        Joseph Boyd added a comment -

        The same fix mentioned in the description works for me.

        Show
        Joseph Boyd added a comment - The same fix mentioned in the description works for me.
        Hide
        Joseph Boyd added a comment -

        In the sqoop mailing list on Sep 13, 2011, Ken Krugler added:

        We've been using code with this mod extensively on Macs and Redhat Enterprise 5.3, so it's been tested in those environments.

        Show
        Joseph Boyd added a comment - In the sqoop mailing list on Sep 13, 2011, Ken Krugler added: We've been using code with this mod extensively on Macs and Redhat Enterprise 5.3, so it's been tested in those environments.
        Hide
        Joey Echeverria added a comment -

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        Show
        Joey Echeverria added a comment - Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Review request for Sqoop.

        Summary
        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.
        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs


        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing
        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- Review request for Sqoop. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Thanks for taking this up Joey. Considering the various platforms that Sqoop is used on, it is likely that this change may cause some backward compatibility issues for some. In order to avoid that, I would suggest conditionally using the mkfifo based on environment setting or command line option.

        Your thoughts?

        • Arvind

        On 2011-09-16 20:38:34, Joey Echeverria wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1938/

        -----------------------------------------------------------

        (Updated 2011-09-16 20:38:34)

        Review request for Sqoop.

        Summary

        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.

        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs

        -----

        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing

        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/#review1944 ----------------------------------------------------------- Thanks for taking this up Joey. Considering the various platforms that Sqoop is used on, it is likely that this change may cause some backward compatibility issues for some. In order to avoid that, I would suggest conditionally using the mkfifo based on environment setting or command line option. Your thoughts? Arvind On 2011-09-16 20:38:34, Joey Echeverria wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-16 20:38:34) Review request for Sqoop. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs ----- src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        Joey Echeverria added a comment -

        Updated patch with conditional logic to only use mkfifo on Mac.

        Show
        Joey Echeverria added a comment - Updated patch with conditional logic to only use mkfifo on Mac.
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2011-09-16 22:14:55.219088)

        Review request for Sqoop.

        Changes
        -------

        Updated with conditional logic.

        Summary
        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.
        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs (updated)


        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing
        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-16 22:14:55.219088) Review request for Sqoop. Changes ------- Updated with conditional logic. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs (updated) src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        Joseph Boyd added a comment -

        Rather than test the OS variable, could we just run the 'mknod' command, and then run the 'mkfifo' command if mknod fails (returns non-zero)? This would allow the fix to work on other (BSD-ish?) systems that may not have mknod with fifo options.

        This is the approach used within gnu coreutils itself (coreutils-8.13/src/copy.c)
        [ http://ftp.gnu.org/gnu/coreutils/coreutils-8.13.tar.gz ]

        I don't have a strong opinion either way, as I'm not at all sure these things are run on non-linux and non-mac os's very often.

        Show
        Joseph Boyd added a comment - Rather than test the OS variable, could we just run the 'mknod' command, and then run the 'mkfifo' command if mknod fails (returns non-zero)? This would allow the fix to work on other (BSD-ish?) systems that may not have mknod with fifo options. This is the approach used within gnu coreutils itself (coreutils-8.13/src/copy.c) [ http://ftp.gnu.org/gnu/coreutils/coreutils-8.13.tar.gz ] I don't have a strong opinion either way, as I'm not at all sure these things are run on non-linux and non-mac os's very often.
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        src/java/com/cloudera/sqoop/io/NamedFifo.java
        <https://reviews.apache.org/r/1938/#comment4430>

        Thanks for making this change. Considering that there is at least one more system where this problem shows up, I suggest that we add an evnironment variable check, something like sqoop.use.mkfifo, which if set turns this behavior.

        • Arvind

        On 2011-09-16 22:14:55, Joey Echeverria wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1938/

        -----------------------------------------------------------

        (Updated 2011-09-16 22:14:55)

        Review request for Sqoop.

        Summary

        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.

        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs

        -----

        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing

        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/#review1950 ----------------------------------------------------------- src/java/com/cloudera/sqoop/io/NamedFifo.java < https://reviews.apache.org/r/1938/#comment4430 > Thanks for making this change. Considering that there is at least one more system where this problem shows up, I suggest that we add an evnironment variable check, something like sqoop.use.mkfifo, which if set turns this behavior. Arvind On 2011-09-16 22:14:55, Joey Echeverria wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-16 22:14:55) Review request for Sqoop. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs ----- src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        Joseph Boyd added a comment -

        I'd be against having to set a 'sqoop.use.mkfifo' env variable ...

        Trying mknod first, then mkfifo, would maintain backwards compatibility, would not require a new env var, and would work on more than just mac os. A win,win,win scenario.

        The Shell.execCommand() static method used now doesn't return the exit code for error checking, but that call is easily replaced with one that does.

        Show
        Joseph Boyd added a comment - I'd be against having to set a 'sqoop.use.mkfifo' env variable ... Trying mknod first, then mkfifo, would maintain backwards compatibility, would not require a new env var, and would work on more than just mac os. A win,win,win scenario. The Shell.execCommand() static method used now doesn't return the exit code for error checking, but that call is easily replaced with one that does.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-09-16 23:17:01, Arvind Prabhakar wrote:

        > src/java/com/cloudera/sqoop/io/NamedFifo.java, line 74

        > <https://reviews.apache.org/r/1938/diff/2/?file=41551#file41551line74>

        >

        > Thanks for making this change. Considering that there is at least one more system where this problem shows up, I suggest that we add an evnironment variable check, something like sqoop.use.mkfifo, which if set turns this behavior.

        I don't like making things require a configuration if we don't need to. How about I just check to see what's in the path? If mknod is there, if not pick mkfifo. If neither is available, throw an error? Less work for the user, works everywhere. I can do the check once when the class is loaded and then just remember my choice.

        • Joey

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

        On 2011-09-16 22:14:55, Joey Echeverria wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1938/

        -----------------------------------------------------------

        (Updated 2011-09-16 22:14:55)

        Review request for Sqoop.

        Summary

        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.

        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs

        -----

        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing

        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-09-16 23:17:01, Arvind Prabhakar wrote: > src/java/com/cloudera/sqoop/io/NamedFifo.java, line 74 > < https://reviews.apache.org/r/1938/diff/2/?file=41551#file41551line74 > > > Thanks for making this change. Considering that there is at least one more system where this problem shows up, I suggest that we add an evnironment variable check, something like sqoop.use.mkfifo, which if set turns this behavior. I don't like making things require a configuration if we don't need to. How about I just check to see what's in the path? If mknod is there, if not pick mkfifo. If neither is available, throw an error? Less work for the user, works everywhere. I can do the check once when the class is loaded and then just remember my choice. Joey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/#review1950 ----------------------------------------------------------- On 2011-09-16 22:14:55, Joey Echeverria wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-16 22:14:55) Review request for Sqoop. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs ----- src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-09-16 23:17:01, Arvind Prabhakar wrote:

        > src/java/com/cloudera/sqoop/io/NamedFifo.java, line 74

        > <https://reviews.apache.org/r/1938/diff/2/?file=41551#file41551line74>

        >

        > Thanks for making this change. Considering that there is at least one more system where this problem shows up, I suggest that we add an evnironment variable check, something like sqoop.use.mkfifo, which if set turns this behavior.

        Joey Echeverria wrote:

        I don't like making things require a configuration if we don't need to. How about I just check to see what's in the path? If mknod is there, if not pick mkfifo. If neither is available, throw an error? Less work for the user, works everywhere. I can do the check once when the class is loaded and then just remember my choice.

        mknod exists on mac os x, it just can't create a FIFO.

        I suggested in the JIRA that we try running mknod and then fall back to running mkfifo if the mknod call returns a non-zero exit code.

        • Joseph

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

        On 2011-09-16 22:14:55, Joey Echeverria wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1938/

        -----------------------------------------------------------

        (Updated 2011-09-16 22:14:55)

        Review request for Sqoop.

        Summary

        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.

        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs

        -----

        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing

        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-09-16 23:17:01, Arvind Prabhakar wrote: > src/java/com/cloudera/sqoop/io/NamedFifo.java, line 74 > < https://reviews.apache.org/r/1938/diff/2/?file=41551#file41551line74 > > > Thanks for making this change. Considering that there is at least one more system where this problem shows up, I suggest that we add an evnironment variable check, something like sqoop.use.mkfifo, which if set turns this behavior. Joey Echeverria wrote: I don't like making things require a configuration if we don't need to. How about I just check to see what's in the path? If mknod is there, if not pick mkfifo. If neither is available, throw an error? Less work for the user, works everywhere. I can do the check once when the class is loaded and then just remember my choice. mknod exists on mac os x, it just can't create a FIFO. I suggested in the JIRA that we try running mknod and then fall back to running mkfifo if the mknod call returns a non-zero exit code. Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/#review1950 ----------------------------------------------------------- On 2011-09-16 22:14:55, Joey Echeverria wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-16 22:14:55) Review request for Sqoop. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs ----- src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-09-16 23:17:01, Arvind Prabhakar wrote:

        > src/java/com/cloudera/sqoop/io/NamedFifo.java, line 74

        > <https://reviews.apache.org/r/1938/diff/2/?file=41551#file41551line74>

        >

        > Thanks for making this change. Considering that there is at least one more system where this problem shows up, I suggest that we add an evnironment variable check, something like sqoop.use.mkfifo, which if set turns this behavior.

        Joey Echeverria wrote:

        I don't like making things require a configuration if we don't need to. How about I just check to see what's in the path? If mknod is there, if not pick mkfifo. If neither is available, throw an error? Less work for the user, works everywhere. I can do the check once when the class is loaded and then just remember my choice.

        Joseph Boyd wrote:

        mknod exists on mac os x, it just can't create a FIFO.

        I suggested in the JIRA that we try running mknod and then fall back to running mkfifo if the mknod call returns a non-zero exit code.

        I saw that after I posted. I could switch the logic and search for mkfifo first. I assume that mkfifo can always make a fifo, right

        • Joey

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

        On 2011-09-16 22:14:55, Joey Echeverria wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1938/

        -----------------------------------------------------------

        (Updated 2011-09-16 22:14:55)

        Review request for Sqoop.

        Summary

        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.

        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs

        -----

        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing

        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-09-16 23:17:01, Arvind Prabhakar wrote: > src/java/com/cloudera/sqoop/io/NamedFifo.java, line 74 > < https://reviews.apache.org/r/1938/diff/2/?file=41551#file41551line74 > > > Thanks for making this change. Considering that there is at least one more system where this problem shows up, I suggest that we add an evnironment variable check, something like sqoop.use.mkfifo, which if set turns this behavior. Joey Echeverria wrote: I don't like making things require a configuration if we don't need to. How about I just check to see what's in the path? If mknod is there, if not pick mkfifo. If neither is available, throw an error? Less work for the user, works everywhere. I can do the check once when the class is loaded and then just remember my choice. Joseph Boyd wrote: mknod exists on mac os x, it just can't create a FIFO. I suggested in the JIRA that we try running mknod and then fall back to running mkfifo if the mknod call returns a non-zero exit code. I saw that after I posted. I could switch the logic and search for mkfifo first. I assume that mkfifo can always make a fifo, right Joey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/#review1950 ----------------------------------------------------------- On 2011-09-16 22:14:55, Joey Echeverria wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-16 22:14:55) Review request for Sqoop. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs ----- src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2011-09-19 01:06:45.395494)

        Review request for Sqoop.

        Changes
        -------

        Here's a version that looks for mkfifo and falls back to mknod if it's not available. This should gracefully handle any os without introducing a new configuration parameter.

        Summary
        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.
        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs (updated)


        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing
        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-19 01:06:45.395494) Review request for Sqoop. Changes ------- Here's a version that looks for mkfifo and falls back to mknod if it's not available. This should gracefully handle any os without introducing a new configuration parameter. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs (updated) src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        Joey Echeverria added a comment -

        Here's a version that looks for mkfifo and falls back to mknod if it's not available. This should gracefully handle any os without introducing a new configuration parameter.

        Show
        Joey Echeverria added a comment - Here's a version that looks for mkfifo and falls back to mknod if it's not available. This should gracefully handle any os without introducing a new configuration parameter.
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Ship it!

        This patch works for me.

        Other's might be concerned that it changes the default behavior to using mkfifo instead of mknod, but my resarch of the differences between them shows little to worry about.

        • Joseph

        On 2011-09-19 01:06:45, Joey Echeverria wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1938/

        -----------------------------------------------------------

        (Updated 2011-09-19 01:06:45)

        Review request for Sqoop.

        Summary

        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.

        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs

        -----

        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing

        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/#review1968 ----------------------------------------------------------- Ship it! This patch works for me. Other's might be concerned that it changes the default behavior to using mkfifo instead of mknod, but my resarch of the differences between them shows little to worry about. Joseph On 2011-09-19 01:06:45, Joey Echeverria wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-19 01:06:45) Review request for Sqoop. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs ----- src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-09-16 21:00:37, Arvind Prabhakar wrote:

        > Thanks for taking this up Joey. Considering the various platforms that Sqoop is used on, it is likely that this change may cause some backward compatibility issues for some. In order to avoid that, I would suggest conditionally using the mkfifo based on environment setting or command line option.

        >

        > Your thoughts?

        Any comments on the latest version?

        • Joey

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

        On 2011-09-19 01:06:45, Joey Echeverria wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1938/

        -----------------------------------------------------------

        (Updated 2011-09-19 01:06:45)

        Review request for Sqoop.

        Summary

        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.

        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs

        -----

        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing

        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-09-16 21:00:37, Arvind Prabhakar wrote: > Thanks for taking this up Joey. Considering the various platforms that Sqoop is used on, it is likely that this change may cause some backward compatibility issues for some. In order to avoid that, I would suggest conditionally using the mkfifo based on environment setting or command line option. > > Your thoughts? Any comments on the latest version? Joey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/#review1944 ----------------------------------------------------------- On 2011-09-19 01:06:45, Joey Echeverria wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-19 01:06:45) Review request for Sqoop. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs ----- src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-09-16 21:00:37, Arvind Prabhakar wrote:

        > Thanks for taking this up Joey. Considering the various platforms that Sqoop is used on, it is likely that this change may cause some backward compatibility issues for some. In order to avoid that, I would suggest conditionally using the mkfifo based on environment setting or command line option.

        >

        > Your thoughts?

        Joey Echeverria wrote:

        Any comments on the latest version?

        Sorry about the delay in responding Joey. I still have the same concern that the default code-path has changed and it could cause regression on systems where Sqoop otherwise has worked in the past. I would therefore suggest that you default to the old way of doing things, and conditionally (implicit or explicit), modify that behavior where necessary.

        • Arvind

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

        On 2011-09-19 01:06:45, Joey Echeverria wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1938/

        -----------------------------------------------------------

        (Updated 2011-09-19 01:06:45)

        Review request for Sqoop.

        Summary

        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.

        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs

        -----

        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing

        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-09-16 21:00:37, Arvind Prabhakar wrote: > Thanks for taking this up Joey. Considering the various platforms that Sqoop is used on, it is likely that this change may cause some backward compatibility issues for some. In order to avoid that, I would suggest conditionally using the mkfifo based on environment setting or command line option. > > Your thoughts? Joey Echeverria wrote: Any comments on the latest version? Sorry about the delay in responding Joey. I still have the same concern that the default code-path has changed and it could cause regression on systems where Sqoop otherwise has worked in the past. I would therefore suggest that you default to the old way of doing things, and conditionally (implicit or explicit), modify that behavior where necessary. Arvind ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/#review1944 ----------------------------------------------------------- On 2011-09-19 01:06:45, Joey Echeverria wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-19 01:06:45) Review request for Sqoop. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs ----- src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-09-16 21:00:37, Arvind Prabhakar wrote:

        > Thanks for taking this up Joey. Considering the various platforms that Sqoop is used on, it is likely that this change may cause some backward compatibility issues for some. In order to avoid that, I would suggest conditionally using the mkfifo based on environment setting or command line option.

        >

        > Your thoughts?

        Joey Echeverria wrote:

        Any comments on the latest version?

        Arvind Prabhakar wrote:

        Sorry about the delay in responding Joey. I still have the same concern that the default code-path has changed and it could cause regression on systems where Sqoop otherwise has worked in the past. I would therefore suggest that you default to the old way of doing things, and conditionally (implicit or explicit), modify that behavior where necessary.

        So, I've got two ideas for how to restructure it to default to mknod:

        1) Try using mknod in the static initializer to create a fifo in /tmp. If it fails, use mkfifo from now on.
        2) Always try using mknod in the create() method, and fall back to mkfifo every time it fails.

        The one downside I see to 1 is it could falsely claim that mknod doesn't work because of a, potentially temporary, environmental issue. Preferences?

        • Joey

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

        On 2011-09-19 01:06:45, Joey Echeverria wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1938/

        -----------------------------------------------------------

        (Updated 2011-09-19 01:06:45)

        Review request for Sqoop.

        Summary

        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.

        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs

        -----

        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing

        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-09-16 21:00:37, Arvind Prabhakar wrote: > Thanks for taking this up Joey. Considering the various platforms that Sqoop is used on, it is likely that this change may cause some backward compatibility issues for some. In order to avoid that, I would suggest conditionally using the mkfifo based on environment setting or command line option. > > Your thoughts? Joey Echeverria wrote: Any comments on the latest version? Arvind Prabhakar wrote: Sorry about the delay in responding Joey. I still have the same concern that the default code-path has changed and it could cause regression on systems where Sqoop otherwise has worked in the past. I would therefore suggest that you default to the old way of doing things, and conditionally (implicit or explicit), modify that behavior where necessary. So, I've got two ideas for how to restructure it to default to mknod: 1) Try using mknod in the static initializer to create a fifo in /tmp. If it fails, use mkfifo from now on. 2) Always try using mknod in the create() method, and fall back to mkfifo every time it fails. The one downside I see to 1 is it could falsely claim that mknod doesn't work because of a, potentially temporary, environmental issue. Preferences? Joey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/#review1944 ----------------------------------------------------------- On 2011-09-19 01:06:45, Joey Echeverria wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-19 01:06:45) Review request for Sqoop. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs ----- src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-09-16 21:00:37, Arvind Prabhakar wrote:

        > Thanks for taking this up Joey. Considering the various platforms that Sqoop is used on, it is likely that this change may cause some backward compatibility issues for some. In order to avoid that, I would suggest conditionally using the mkfifo based on environment setting or command line option.

        >

        > Your thoughts?

        Joey Echeverria wrote:

        Any comments on the latest version?

        Arvind Prabhakar wrote:

        Sorry about the delay in responding Joey. I still have the same concern that the default code-path has changed and it could cause regression on systems where Sqoop otherwise has worked in the past. I would therefore suggest that you default to the old way of doing things, and conditionally (implicit or explicit), modify that behavior where necessary.

        Joey Echeverria wrote:

        So, I've got two ideas for how to restructure it to default to mknod:

        1) Try using mknod in the static initializer to create a fifo in /tmp. If it fails, use mkfifo from now on.

        2) Always try using mknod in the create() method, and fall back to mkfifo every time it fails.

        The one downside I see to 1 is it could falsely claim that mknod doesn't work because of a, potentially temporary, environmental issue. Preferences?

        I think #2 would work best, with some log message indicating that the default option has failed and trying a the mkfifo instead. Failure can be detected by stating the fifo file to see if it exists.

        While you are modifying that part of the code, it will also be good to log the command output which is currently being ignored.

        • Arvind

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

        On 2011-09-19 01:06:45, Joey Echeverria wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1938/

        -----------------------------------------------------------

        (Updated 2011-09-19 01:06:45)

        Review request for Sqoop.

        Summary

        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.

        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs

        -----

        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing

        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-09-16 21:00:37, Arvind Prabhakar wrote: > Thanks for taking this up Joey. Considering the various platforms that Sqoop is used on, it is likely that this change may cause some backward compatibility issues for some. In order to avoid that, I would suggest conditionally using the mkfifo based on environment setting or command line option. > > Your thoughts? Joey Echeverria wrote: Any comments on the latest version? Arvind Prabhakar wrote: Sorry about the delay in responding Joey. I still have the same concern that the default code-path has changed and it could cause regression on systems where Sqoop otherwise has worked in the past. I would therefore suggest that you default to the old way of doing things, and conditionally (implicit or explicit), modify that behavior where necessary. Joey Echeverria wrote: So, I've got two ideas for how to restructure it to default to mknod: 1) Try using mknod in the static initializer to create a fifo in /tmp. If it fails, use mkfifo from now on. 2) Always try using mknod in the create() method, and fall back to mkfifo every time it fails. The one downside I see to 1 is it could falsely claim that mknod doesn't work because of a, potentially temporary, environmental issue. Preferences? I think #2 would work best, with some log message indicating that the default option has failed and trying a the mkfifo instead. Failure can be detected by stating the fifo file to see if it exists. While you are modifying that part of the code, it will also be good to log the command output which is currently being ignored. Arvind ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/#review1944 ----------------------------------------------------------- On 2011-09-19 01:06:45, Joey Echeverria wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-19 01:06:45) Review request for Sqoop. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs ----- src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2011-09-21 17:13:30.493594)

        Review request for Sqoop.

        Changes
        -------

        Incorporated the latest feedback. This patch uses mknod, and then falls back to mkfifo if the fifo wasn't actually created. The output from both commands is logged.

        Summary
        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.
        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs (updated)


        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing
        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-21 17:13:30.493594) Review request for Sqoop. Changes ------- Incorporated the latest feedback. This patch uses mknod, and then falls back to mkfifo if the fifo wasn't actually created. The output from both commands is logged. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs (updated) src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Code change looks good. On my linux system I tested it out by removing (1) mknod completely from the path, (2) keeping it but revoking the execute permissions, (3) replacing it with a dummy script that does nothing, and (4) replacing it with a dummy script that returned an non-zero exit code. In all but (3), the test failed since the Shell.execCommand() raised an exception (IOException in cases 1,2 and Shell$ExitCodeException in 4). This implies that if the mknod command fails, the routine will exit with error rather than retry it.

        One way to address it would be to put the Shell.exec in try/catch and log the first exception before retry. If it fails again with mkfifo, the exception can bubble up to the caller.

        • Arvind

        On 2011-09-21 17:13:30, Joey Echeverria wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1938/

        -----------------------------------------------------------

        (Updated 2011-09-21 17:13:30)

        Review request for Sqoop.

        Summary

        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.

        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs

        -----

        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing

        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/#review2016 ----------------------------------------------------------- Code change looks good. On my linux system I tested it out by removing (1) mknod completely from the path, (2) keeping it but revoking the execute permissions, (3) replacing it with a dummy script that does nothing, and (4) replacing it with a dummy script that returned an non-zero exit code. In all but (3), the test failed since the Shell.execCommand() raised an exception (IOException in cases 1,2 and Shell$ExitCodeException in 4). This implies that if the mknod command fails, the routine will exit with error rather than retry it. One way to address it would be to put the Shell.exec in try/catch and log the first exception before retry. If it fails again with mkfifo, the exception can bubble up to the caller. Arvind On 2011-09-21 17:13:30, Joey Echeverria wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-21 17:13:30) Review request for Sqoop. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs ----- src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2011-09-21 23:42:24.872739)

        Review request for Sqoop.

        Changes
        -------

        I added a try/catch around the call to mknod. If an exception is thrown, it will fall back to mkfifo. If mkfifo throws, then that's thrown to the caller. I also ran the 4 tests that Arvind did and all four passed.

        Summary
        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.
        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs (updated)


        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing
        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-21 23:42:24.872739) Review request for Sqoop. Changes ------- I added a try/catch around the call to mknod. If an exception is thrown, it will fall back to mkfifo. If mkfifo throws, then that's thrown to the caller. I also ran the 4 tests that Arvind did and all four passed. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs (updated) src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        (Updated 2011-09-22 00:10:32.121501)

        Review request for Sqoop.

        Changes
        -------

        Missed two checkstyle issues with my last patch, fixed.

        Summary
        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.
        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs (updated)


        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing
        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-22 00:10:32.121501) Review request for Sqoop. Changes ------- Missed two checkstyle issues with my last patch, fixed. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs (updated) src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        Arvind Prabhakar added a comment -

        Patch committed. Thanks Joey!

        Show
        Arvind Prabhakar added a comment - Patch committed. Thanks Joey!
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Ship it!

        +1

        • Arvind

        On 2011-09-22 00:10:32, Joey Echeverria wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/1938/

        -----------------------------------------------------------

        (Updated 2011-09-22 00:10:32)

        Review request for Sqoop.

        Summary

        -------

        Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X.

        This addresses bug SQOOP-339.

        https://issues.apache.org/jira/browse/SQOOP-339

        Diffs

        -----

        src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb

        Diff: https://reviews.apache.org/r/1938/diff

        Testing

        -------

        No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit.

        Thanks,

        Joey

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/#review2017 ----------------------------------------------------------- Ship it! +1 Arvind On 2011-09-22 00:10:32, Joey Echeverria wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1938/ ----------------------------------------------------------- (Updated 2011-09-22 00:10:32) Review request for Sqoop. Summary ------- Replaced the use of mknod with mkfifo in NamedFifo to improve compatibility with Mac OS X. This addresses bug SQOOP-339 . https://issues.apache.org/jira/browse/SQOOP-339 Diffs ----- src/java/com/cloudera/sqoop/io/NamedFifo.java 38656cb Diff: https://reviews.apache.org/r/1938/diff Testing ------- No new tests as the functionality is covered by an existing unit test. I ran the existing unit test and it worked. I don't have a Mac to test on right now, so it might be good to run the unit tests on one before commit. Thanks, Joey
        Hide
        Hudson added a comment -

        Integrated in Sqoop-jdk-1.6 #28 (See https://builds.apache.org/job/Sqoop-jdk-1.6/28/)
        SQOOP-339. Error handling for mknod failure.

        (Joey Echeverria via Arvind Prabhakar)

        arvind : http://svn.apache.org/viewvc/?view=rev&rev=1173919
        Files :

        • /incubator/sqoop/trunk/src/java/com/cloudera/sqoop/io/NamedFifo.java
        Show
        Hudson added a comment - Integrated in Sqoop-jdk-1.6 #28 (See https://builds.apache.org/job/Sqoop-jdk-1.6/28/ ) SQOOP-339 . Error handling for mknod failure. (Joey Echeverria via Arvind Prabhakar) arvind : http://svn.apache.org/viewvc/?view=rev&rev=1173919 Files : /incubator/sqoop/trunk/src/java/com/cloudera/sqoop/io/NamedFifo.java
        Hide
        Joseph Boyd added a comment -

        This patch works for me as well, thx.

        Show
        Joseph Boyd added a comment - This patch works for me as well, thx.

          People

          • Assignee:
            Joey Echeverria
            Reporter:
            Joseph Boyd
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development