Uploaded image for project: 'Apache Tez'
  1. Apache Tez
  2. TEZ-1836

Provide better error messages when tez.runtime.io.sort.mb is wrongly configured

    Details

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

      Description

      For tez.runtime.io.sort.mb=3000, following error message is thrown
      >>>
      info=[Error: Failure while running task:java.lang.RuntimeException: java.io.IOException: Invalid "tez.runtime.io.sort.mb": 3000
      >>>

      This should mention the range of valid configuration for "tez.runtime.io.sort.mb" (1 to 2047)

      For tez.runtime.io.sort.mb=0, following error message is thrown
      >>>>
      info=[Error: Failure while running task:java.lang.IllegalArgumentException: io.sort.mb should be larger than 0
      at com.google.common.base.Preconditions.checkArgument(Preconditions.java:88)
      at org.apache.tez.runtime.library.common.sort.impl.ExternalSorter.getInitialMemoryRequirement(ExternalSorter.java:289)
      >>>>

      This should mention "tez.runtime.io.sort.mb" and not "io.sort.mb".

      1. TEZ-1836.3.patch
        8 kB
        Vasanth kumar RJ
      2. TEZ-1836.3.2.txt
        8 kB
        Siddharth Seth
      3. TEZ-1836.2.patch
        8 kB
        Vasanth kumar RJ
      4. TEZ-1836.1.patch
        5 kB
        Vasanth kumar RJ

        Issue Links

          Activity

          Hide
          vasanthkumar Vasanth kumar RJ added a comment -

          Rajesh Balamohan, Attached patch. Please review.

          Show
          vasanthkumar Vasanth kumar RJ added a comment - Rajesh Balamohan , Attached patch. Please review.
          Hide
          hitesh Hitesh Shah added a comment -

          Siddharth Seth Mind taking a look? I have a question on what should be our check. Earlier, there were redundant checks on both the assignedMemory as well as the configured memory. My understanding is that it should likely be safe to check only the initial configured memory ( the assumption being that if there is enough memory in the JVM, the assigned value will be the same as the configured one in both single and multiple sorter scenarios ).

          Vasanth kumar RJ Thanks for the patch. An additional minor nit to be addressed - the exception thrown for TezRuntimeConfiguration.TEZ_RUNTIME_SORT_SPILL_PERCENT needs a similar clarification fix.

          Show
          hitesh Hitesh Shah added a comment - Siddharth Seth Mind taking a look? I have a question on what should be our check. Earlier, there were redundant checks on both the assignedMemory as well as the configured memory. My understanding is that it should likely be safe to check only the initial configured memory ( the assumption being that if there is enough memory in the JVM, the assigned value will be the same as the configured one in both single and multiple sorter scenarios ). Vasanth kumar RJ Thanks for the patch. An additional minor nit to be addressed - the exception thrown for TezRuntimeConfiguration.TEZ_RUNTIME_SORT_SPILL_PERCENT needs a similar clarification fix.
          Hide
          vasanthkumar Vasanth kumar RJ added a comment -

          Hitesh Shah, implemented review comments and attached patch

          Show
          vasanthkumar Vasanth kumar RJ added a comment - Hitesh Shah , implemented review comments and attached patch
          Hide
          sseth Siddharth Seth added a comment -

          Vasanth kumar RJ - thanks for posting the patches. They look good to me. Minor.
          spillPer is only used in DefaultSorter - instead of moving the check to ExternalSorter, could you move it into DefaultSorter only.

          The other question is what we should do when io.sort.mb is configured to a higher value - throw an error, or just reduce it to the maximum allowed value. The PipelineSorter will soon support > 2GB buffers - in which case, I think it's OK to just reduce it in case of the DefaultSorter with a warning. We could do that as part of the PipelineSorter patch though - since the current behaviour is to throw an error on a buffer higher than 2GB.

          In terms of checking at request time / assign time. Checking at assign time would guard against bugs in the MemoryDisrtibutor - in case they give back more than what was requested. Don't think it's absolutely mandatory.

          Show
          sseth Siddharth Seth added a comment - Vasanth kumar RJ - thanks for posting the patches. They look good to me. Minor. spillPer is only used in DefaultSorter - instead of moving the check to ExternalSorter, could you move it into DefaultSorter only. The other question is what we should do when io.sort.mb is configured to a higher value - throw an error, or just reduce it to the maximum allowed value. The PipelineSorter will soon support > 2GB buffers - in which case, I think it's OK to just reduce it in case of the DefaultSorter with a warning. We could do that as part of the PipelineSorter patch though - since the current behaviour is to throw an error on a buffer higher than 2GB. In terms of checking at request time / assign time. Checking at assign time would guard against bugs in the MemoryDisrtibutor - in case they give back more than what was requested. Don't think it's absolutely mandatory.
          Hide
          hitesh Hitesh Shah added a comment -

          Cancelling patch for now as there are review comments to be addressed.

          Show
          hitesh Hitesh Shah added a comment - Cancelling patch for now as there are review comments to be addressed.
          Hide
          vasanthkumar Vasanth kumar RJ added a comment -

          Siddharth Seth, Implemented review comments.
          But spillPer is used in DefaultSorter and PipelinedSorter constructor for pre-check, so I moved to ExternalSorter.

          Show
          vasanthkumar Vasanth kumar RJ added a comment - Siddharth Seth , Implemented review comments. But spillPer is used in DefaultSorter and PipelinedSorter constructor for pre-check, so I moved to ExternalSorter.
          Hide
          sseth Siddharth Seth added a comment -

          spillPer isn't used at all in PipelineSorter, so it can be completely removed - including setting it up. I'm going to upload a quick patch on top of yours to remove the lines from PipelineSorter, and commit this. Thanks Vasanth kumar RJ.

          Show
          sseth Siddharth Seth added a comment - spillPer isn't used at all in PipelineSorter, so it can be completely removed - including setting it up. I'm going to upload a quick patch on top of yours to remove the lines from PipelineSorter, and commit this. Thanks Vasanth kumar RJ .
          Hide
          sseth Siddharth Seth added a comment -

          Revision of the .3 patch which removed spillPer from pipeline sorter.

          Show
          sseth Siddharth Seth added a comment - Revision of the .3 patch which removed spillPer from pipeline sorter.
          Hide
          sseth Siddharth Seth added a comment -

          Committed to master and branch-0.5.

          Show
          sseth Siddharth Seth added a comment - Committed to master and branch-0.5.
          Hide
          sseth Siddharth Seth added a comment - - edited

          The modification broke the test (i.e. removing all references to spillPer in PipelineSorter - .3.2). Fixing this via a new jira.

          Show
          sseth Siddharth Seth added a comment - - edited The modification broke the test (i.e. removing all references to spillPer in PipelineSorter - .3.2). Fixing this via a new jira.
          Hide
          hitesh Hitesh Shah added a comment -

          Closing issue as 0.5.4, 0.6.1 and 0.7.0 have been released.

          Show
          hitesh Hitesh Shah added a comment - Closing issue as 0.5.4, 0.6.1 and 0.7.0 have been released.

            People

            • Assignee:
              vasanthkumar Vasanth kumar RJ
              Reporter:
              rajesh.balamohan Rajesh Balamohan
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development