Pig
  1. Pig
  2. PIG-3874

FileLocalizer temp path can sometimes be non-unique

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.13.0
    • Fix Version/s: 0.13.0
    • Component/s: None
    • Labels:
      None

      Description

      In some rare corner cases, more than one process can arrive at the same randomly generated temporary path to localize task files. This needs to be handled with a check to see if location already exists and to get a unique path.

      1. PIG-3874.patch
        5 kB
        Mona Chitnis
      2. PIG-3874-1.patch
        5 kB
        Mona Chitnis
      3. PIG-3874-2.patch
        6 kB
        Mona Chitnis
      4. PIG-3874-3.patch
        5 kB
        Mona Chitnis
      5. PIG-3874-4.patch
        5 kB
        Cheolsoo Park

        Issue Links

          Activity

          Hide
          Cheolsoo Park added a comment -

          Committed to trunk and branch-0.13. Thank you Mona!

          Show
          Cheolsoo Park added a comment - Committed to trunk and branch-0.13. Thank you Mona!
          Hide
          Cheolsoo Park added a comment -

          Mona Chitnis, I think you have a mistake in the last patch.

          -    private static void verifyStringContained(List<URL> list, String name, boolean included) {
          +    private static void verifyStringContained(Set<URL> list, String name, boolean included) {
          

          This changes breaks compilation in the following lines of TestPigServer.java-

          verifyStringContained(pig.getPigContext().extraJars, jarName, false); // extraJars is List not Set
          

          I am attaching a new patch that fixes this error and will commit this.

          Show
          Cheolsoo Park added a comment - Mona Chitnis , I think you have a mistake in the last patch. - private static void verifyStringContained(List<URL> list, String name, boolean included) { + private static void verifyStringContained(Set<URL> list, String name, boolean included) { This changes breaks compilation in the following lines of TestPigServer.java- verifyStringContained(pig.getPigContext().extraJars, jarName, false ); // extraJars is List not Set I am attaching a new patch that fixes this error and will commit this.
          Hide
          Mona Chitnis added a comment -

          rebased patch to updated trunk. checked unit test in patch passes

          Show
          Mona Chitnis added a comment - rebased patch to updated trunk. checked unit test in patch passes
          Hide
          Aniket Mokashi added a comment -

          Missed the while loop in the patch. LGTM now. +1

          Show
          Aniket Mokashi added a comment - Missed the while loop in the patch. LGTM now. +1
          Hide
          Aniket Mokashi added a comment -

          Patch needs to be rebased.
          Why not try n-times instead of two- that will also avoid code duplication.

          Something like-

          int retryCount = 0;
          Exception caughtException = null;
          while (true) {
            try {
               // createRelativeRoot etc.
               // and then
               break;
             } catch (IOException ioe) {
               caughtException = ioe;
             }
             if (retryCount >= retryLimit) { // retryLimit = 5 hardcoded
               throw new IOException(caughtException);
             }
             retryCount++;
             //Thread.sleep() etc.
          }
          
          Show
          Aniket Mokashi added a comment - Patch needs to be rebased. Why not try n-times instead of two- that will also avoid code duplication. Something like- int retryCount = 0; Exception caughtException = null ; while ( true ) { try { // createRelativeRoot etc. // and then break ; } catch (IOException ioe) { caughtException = ioe; } if (retryCount >= retryLimit) { // retryLimit = 5 hardcoded throw new IOException(caughtException); } retryCount++; // Thread .sleep() etc. }
          Hide
          Mona Chitnis added a comment -

          updated patch PIG-3874-2.patch after review comment

          Show
          Mona Chitnis added a comment - updated patch PIG-3874 -2.patch after review comment
          Hide
          Rohini Palaniswamy added a comment -

          Can you simplify

          String tempPath= FileLocalizer.getTemporaryPath(pigContext).toString();
                  Path path = new Path(tempPath);
                  URI uri = path.toUri();
                  String prefix = "";
                  if (uri.getScheme() != null) {
                      prefix = uri.getScheme() + ":";
                  }
                  assertTrue(tempPath.startsWith(prefix + pigTempDir.getPath()));
          

          to

          String tempPath= FileLocalizer.getTemporaryPath(pigContext).toString();
          Path path = new Path(tempPath);
          assertTrue(tempPath.startsWith(pigTempDir.toURI()));
          
          Show
          Rohini Palaniswamy added a comment - Can you simplify String tempPath= FileLocalizer.getTemporaryPath(pigContext).toString(); Path path = new Path(tempPath); URI uri = path.toUri(); String prefix = ""; if (uri.getScheme() != null ) { prefix = uri.getScheme() + ":" ; } assertTrue(tempPath.startsWith(prefix + pigTempDir.getPath())); to String tempPath= FileLocalizer.getTemporaryPath(pigContext).toString(); Path path = new Path(tempPath); assertTrue(tempPath.startsWith(pigTempDir.toURI()));
          Hide
          Mona Chitnis added a comment -

          updated the patch to account for Windows filesystem

          Show
          Mona Chitnis added a comment - updated the patch to account for Windows filesystem
          Hide
          Mona Chitnis added a comment -

          Patch attached

          Show
          Mona Chitnis added a comment - Patch attached

            People

            • Assignee:
              Mona Chitnis
              Reporter:
              Mona Chitnis
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development