Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.8.0-alpha
    • Component/s: None
    • Labels:
      None

      Description

      When a Tez Job is submitted via TezClient, an ApplicationSubmissionContext is created before submitting the job. ApplicationSubmissionContext has a priority field which can be used to provide a priority for the job.

      There is an ongoing effort in the Yarn Community to enable application priorities(https://issues.apache.org/jira/browse/YARN-1963).

      https://issues.apache.org/jira/browse/YARN-2003 implements the necessary changes in RM and Capacity Scheduler.

      1. TEZ-2627.1.patch
        5 kB
        Saikat
      2. TEZ-2627.2.patch
        7 kB
        Saikat
      3. TEZ-2627.3.patch
        7 kB
        Saikat
      4. TEZ-2627.4.patch
        7 kB
        Saikat
      5. TEZ-2627.5.modified.patch
        7 kB
        Hitesh Shah
      6. TEZ-2627.5.patch
        7 kB
        Saikat
      7. TEZ-2627.patch
        5 kB
        Saikat

        Issue Links

          Activity

          Hide
          saikatr Saikat added a comment - - edited

          Adding Jonathan Eagles Rajesh Balamohan Eric Payne
          for comments and watch.

          Show
          saikatr Saikat added a comment - - edited Adding Jonathan Eagles Rajesh Balamohan Eric Payne for comments and watch.
          Hide
          saikatr Saikat added a comment - - edited

          Rajesh Balamohan Hitesh Shah
          YARN-2003 got merged in hadoop trunk.
          Can you please comment on the following approach:
          We can create a new configuration tez.job.priority(default value of 0, based on YARN-2003).

          In TezClientUtils.java, createApplicationSubmissionContext(), we can read the configuration(which can be hard coded or passed as CL arguement), to provide a job priority to the submitted DAG.
          The path for both session and non session mode, goes through this method before submitting an application.

          example usage:
          hadoop jar tez_extract/tez-examples-0.8.0-SNAPSHOT.jar orderedwordcount -Dtez.queue.name=<queuename> -Dtez.job.priority=4 <ip> <op>

          Show
          saikatr Saikat added a comment - - edited Rajesh Balamohan Hitesh Shah YARN-2003 got merged in hadoop trunk. Can you please comment on the following approach: We can create a new configuration tez.job.priority(default value of 0, based on YARN-2003 ). In TezClientUtils.java, createApplicationSubmissionContext(), we can read the configuration(which can be hard coded or passed as CL arguement), to provide a job priority to the submitted DAG. The path for both session and non session mode, goes through this method before submitting an application. example usage: hadoop jar tez_extract/tez-examples-0.8.0-SNAPSHOT.jar orderedwordcount -Dtez.queue.name=<queuename> -Dtez.job.priority=4 <ip> <op>
          Hide
          hitesh Hitesh Shah added a comment -

          Is the setPriority api on AppSubmissionContext present in all versions of hadoop > 2.2.0?
          If yes, then a new config, "tez.am.app-priority" or simply "tez.am.priority" should be good enough to be introduced and passed through to the app submission context creation layer. If not, a shim will be needed.

          Are there any valid ranges for the priority? Or does YARN throw back an error if there is an issue? Does that error handling need to be special cased or is the exception thrown back clear enough that it can be sent back to the user?

          Show
          hitesh Hitesh Shah added a comment - Is the setPriority api on AppSubmissionContext present in all versions of hadoop > 2.2.0? If yes, then a new config, "tez.am.app-priority" or simply "tez.am.priority" should be good enough to be introduced and passed through to the app submission context creation layer. If not, a shim will be needed. Are there any valid ranges for the priority? Or does YARN throw back an error if there is an issue? Does that error handling need to be special cased or is the exception thrown back clear enough that it can be sent back to the user?
          Hide
          saikatr Saikat added a comment - - edited

          Hitesh Shah
          According to the patch YARN-2003,
          1.there is a new configuration "yarn.cluster.max-application-priority"(default value is 0) and a default application priority per queue(e.g "yarn.scheduler.capacity.<queuename>.default-application-priority" which also has a default value 0).
          If the submitted app, doesnot set the priority field, then the default priority of the queue is taken as the app priority.
          If the submitted app has a set priority, it is compared against the max cluster level app priority and capped if needed.

          2. setPriority() is available for versions > 2.2.0

          The author of YARN-2003 Sunil G has confirmed about this behaviour.

          Show
          saikatr Saikat added a comment - - edited Hitesh Shah According to the patch YARN-2003 , 1.there is a new configuration "yarn.cluster.max-application-priority"(default value is 0) and a default application priority per queue(e.g "yarn.scheduler.capacity.<queuename>.default-application-priority" which also has a default value 0). If the submitted app, doesnot set the priority field, then the default priority of the queue is taken as the app priority. If the submitted app has a set priority, it is compared against the max cluster level app priority and capped if needed. 2. setPriority() is available for versions > 2.2.0 The author of YARN-2003 Sunil G has confirmed about this behaviour.
          Hide
          hitesh Hitesh Shah added a comment - - edited

          Thanks Saikat.

          If the submitted app has a set priority

          I am assuming one can set priority to -1 too or is that changed to 0 by YARN?

          Show
          hitesh Hitesh Shah added a comment - - edited Thanks Saikat . If the submitted app has a set priority I am assuming one can set priority to -1 too or is that changed to 0 by YARN?
          Hide
          saikatr Saikat added a comment -

          We can set negative priorities, but it will be capped to yarn.cluster.max-application-priority.
          for example is we submit with priority -2 but yarn.cluster.max-application-priority = -1 then we will be capped to -1.(lower the number higher the priority)

          Show
          saikatr Saikat added a comment - We can set negative priorities, but it will be capped to yarn.cluster.max-application-priority. for example is we submit with priority -2 but yarn.cluster.max-application-priority = -1 then we will be capped to -1.(lower the number higher the priority)
          Hide
          tezqa TezQA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12746817/TEZ-2618.patch
          against master revision 19fb440.

          -1 patch. master compilation may be broken.

          Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/911//console

          This message is automatically generated.

          Show
          tezqa TezQA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12746817/TEZ-2618.patch against master revision 19fb440. -1 patch . master compilation may be broken. Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/911//console This message is automatically generated.
          Hide
          tezqa TezQA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12746818/TEZ-2627.patch
          against master revision 19fb440.

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

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

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

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

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

          Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/912//testReport/
          Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/912//console

          This message is automatically generated.

          Show
          tezqa TezQA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12746818/TEZ-2627.patch against master revision 19fb440. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/912//testReport/ Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/912//console This message is automatically generated.
          Hide
          saikatr Saikat added a comment -

          Hitesh Shah request you to take a look at the patch.

          Show
          saikatr Saikat added a comment - Hitesh Shah request you to take a look at the patch.
          Hide
          tezqa TezQA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12746822/TEZ-2627.1.patch
          against master revision 19fb440.

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

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

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

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

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

          Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/913//testReport/
          Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/913//console

          This message is automatically generated.

          Show
          tezqa TezQA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12746822/TEZ-2627.1.patch against master revision 19fb440. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/913//testReport/ Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/913//console This message is automatically generated.
          Hide
          hitesh Hitesh Shah added a comment -

          Comments:

          • do not set priority if not defined in config. i.e. TEZ_APPLICATION_PRIORITY_DEFAULT should not exist. Let YARN figure out what it wants to do if an app priority is not provided.
          • Minor nit on the config name: lets call it "tez.am.application.priority" - "tez.am" denotes AM specific value and "application.priority" is pretty clear to understand by itself. Field name should change to TEZ_AM_APPLICATION_PRIORITY as a result.
          • For the unit tests, why are IOException and YARNExceptions being caught? Shouldn't the test fail if an exception is thrown?
          • Might be useful to update say TestTezJobs to set app priority to +1 for one of the jobs that runs against MiniYARNCluster ( not all of the jobs though so that both cases are covered ).
          Show
          hitesh Hitesh Shah added a comment - Comments: do not set priority if not defined in config. i.e. TEZ_APPLICATION_PRIORITY_DEFAULT should not exist. Let YARN figure out what it wants to do if an app priority is not provided. Minor nit on the config name: lets call it "tez.am.application.priority" - "tez.am" denotes AM specific value and "application.priority" is pretty clear to understand by itself. Field name should change to TEZ_AM_APPLICATION_PRIORITY as a result. For the unit tests, why are IOException and YARNExceptions being caught? Shouldn't the test fail if an exception is thrown? Might be useful to update say TestTezJobs to set app priority to +1 for one of the jobs that runs against MiniYARNCluster ( not all of the jobs though so that both cases are covered ).
          Hide
          saikatr Saikat added a comment -

          Addressed review comments by Hitesh Shah

          Show
          saikatr Saikat added a comment - Addressed review comments by Hitesh Shah
          Hide
          hitesh Hitesh Shah added a comment -

          A simpler approach to the isSet() question would be just to use the Configuration::get(String propertyName) API to check whether an entry exists or not and then invoke getInt and setPriority only if the value is non-null. This does mean that the new api in AMConfiguration is no longer needed.

          Show
          hitesh Hitesh Shah added a comment - A simpler approach to the isSet() question would be just to use the Configuration::get(String propertyName) API to check whether an entry exists or not and then invoke getInt and setPriority only if the value is non-null. This does mean that the new api in AMConfiguration is no longer needed.
          Hide
          hitesh Hitesh Shah added a comment -

          Additional nit: for adding a log message for prio, it will be useful to log the applicationId at the same time. When used in daemons such as HiveServer2, not having identifiers makes the logs hard to understand.

          Show
          hitesh Hitesh Shah added a comment - Additional nit: for adding a log message for prio, it will be useful to log the applicationId at the same time. When used in daemons such as HiveServer2, not having identifiers makes the logs hard to understand.
          Hide
          saikatr Saikat added a comment -

          Hitesh Shah Thanks for your review comments. I had thought about the null check approach.

          If the get() returns non null, then I would still need to pass a default invalid value to getInt() to check.
          And I need the api getPriority in AMConfiguration since yarnconfig is private to the class.
          So, I had implemented it the way as in patch2, since getInt internally does the null check.
          Can you please comment as to how you are expecting the isset() to look like. I might be missing a point.

          for the second comment, I will make the change for logging priority with appid.

          Show
          saikatr Saikat added a comment - Hitesh Shah Thanks for your review comments. I had thought about the null check approach. If the get() returns non null, then I would still need to pass a default invalid value to getInt() to check. And I need the api getPriority in AMConfiguration since yarnconfig is private to the class. So, I had implemented it the way as in patch2, since getInt internally does the null check. Can you please comment as to how you are expecting the isset() to look like. I might be missing a point. for the second comment, I will make the change for logging priority with appid.
          Hide
          hitesh Hitesh Shah added a comment -

          I was thinking of code changing only in TezClientUtils. Something along these lines:

              if ( amConf.getTezConf().get(AM_PRIO) != null) { // isEmpty() check needed if Integer.parseInt used
                   int prio = amConf.getTezConf().get(AM_PRIO, 0); // 0 will never be used as conf string is non-null 
                   submissionCtxt.setPriority(prio);
              }
          

          There are a couple of things to note - Configuration::getInt() only uses the default if prop value is null. The code above could be changed to use Integer.parseInt() instead of relying impl behavior of Conf::getInt().

          Show
          hitesh Hitesh Shah added a comment - I was thinking of code changing only in TezClientUtils. Something along these lines: if ( amConf.getTezConf().get(AM_PRIO) != null ) { // isEmpty() check needed if Integer .parseInt used int prio = amConf.getTezConf().get(AM_PRIO, 0); // 0 will never be used as conf string is non- null submissionCtxt.setPriority(prio); } There are a couple of things to note - Configuration::getInt() only uses the default if prop value is null. The code above could be changed to use Integer.parseInt() instead of relying impl behavior of Conf::getInt().
          Hide
          saikatr Saikat added a comment -

          ok got it. Will submit a patch as suggested

          Show
          saikatr Saikat added a comment - ok got it. Will submit a patch as suggested
          Hide
          tezqa TezQA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12747032/TEZ-2627.3.patch
          against master revision 19fb440.

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

          +1 tests included. The patch appears to include 2 new or modified test files.

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

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

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

          Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/920//testReport/
          Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/920//console

          This message is automatically generated.

          Show
          tezqa TezQA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12747032/TEZ-2627.3.patch against master revision 19fb440. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/920//testReport/ Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/920//console This message is automatically generated.
          Hide
          saikatr Saikat added a comment - - edited

          Hitesh Shah submitted patchset 3 according to your latest review comments.

          Show
          saikatr Saikat added a comment - - edited Hitesh Shah submitted patchset 3 according to your latest review comments.
          Hide
          hitesh Hitesh Shah added a comment -

          Comments:

          • the log debug message in the createAppSubmCtxt function can likely be removed and moved to the setApplicationPriority function. Something simple like "Settting application priority, applicationId=<appId>, priority=<priority>"
          • add a comment to explain why 0 will not be used in "int priority = amConfig.getTezConfiguration().getInt(TezConfiguration.TEZ_AM_APPLICATION_PRIORITY,0);"
          • setApplicationPriority should have a VisibleForTesting annotation
          • rename testAppSubmissionContext to something more relevant to what it is doing i.e. testing app priority. Given that a call to YARN is not being made, a single priority value check ( +999 ) or something should suffice.
          • not sure why a compareTo is needed to compare 2 ints for the priority comparison. Maybe just use Priority::getPriority() which is an int?
          • Simpler to just use "TezConfiguration conf = new TezConfiguration(false);" instead of a mock.
          • Likewise for AMConfiguration.
          • TestTezJobs.java code change should use a non-zero value for priority
          Show
          hitesh Hitesh Shah added a comment - Comments: the log debug message in the createAppSubmCtxt function can likely be removed and moved to the setApplicationPriority function. Something simple like "Settting application priority, applicationId=<appId>, priority=<priority>" add a comment to explain why 0 will not be used in "int priority = amConfig.getTezConfiguration().getInt(TezConfiguration.TEZ_AM_APPLICATION_PRIORITY,0);" setApplicationPriority should have a VisibleForTesting annotation rename testAppSubmissionContext to something more relevant to what it is doing i.e. testing app priority. Given that a call to YARN is not being made, a single priority value check ( +999 ) or something should suffice. not sure why a compareTo is needed to compare 2 ints for the priority comparison. Maybe just use Priority::getPriority() which is an int? Simpler to just use "TezConfiguration conf = new TezConfiguration(false);" instead of a mock. Likewise for AMConfiguration. TestTezJobs.java code change should use a non-zero value for priority
          Hide
          saikatr Saikat added a comment -

          Hitesh Shah addressed all your review comments. Thanks.

          Show
          saikatr Saikat added a comment - Hitesh Shah addressed all your review comments. Thanks.
          Hide
          hitesh Hitesh Shah added a comment -

          Mostly looks good. A couple of minor nits:

          • unused import - import static org.mockito.Mockito.doReturn;
          • use assertEquals and assertNull instead of assertTrue in the new tests.
          Show
          hitesh Hitesh Shah added a comment - Mostly looks good. A couple of minor nits: unused import - import static org.mockito.Mockito.doReturn; use assertEquals and assertNull instead of assertTrue in the new tests.
          Hide
          saikatr Saikat added a comment -

          Done. Thanks for the detailed review Hitesh Shah

          Show
          saikatr Saikat added a comment - Done. Thanks for the detailed review Hitesh Shah
          Hide
          hitesh Hitesh Shah added a comment -

          Thanks for patiently addressing the comments Saikat. Made a minor change to not catch the exceptions and assert.fail(). Will wait for jenkins before committing.

          Show
          hitesh Hitesh Shah added a comment - Thanks for patiently addressing the comments Saikat . Made a minor change to not catch the exceptions and assert.fail(). Will wait for jenkins before committing.
          Hide
          tezqa TezQA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12747404/TEZ-2627.4.patch
          against master revision b9d5c20.

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

          +1 tests included. The patch appears to include 2 new or modified test files.

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

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

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

          Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/926//testReport/
          Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/926//console

          This message is automatically generated.

          Show
          tezqa TezQA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12747404/TEZ-2627.4.patch against master revision b9d5c20. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/926//testReport/ Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/926//console This message is automatically generated.
          Hide
          tezqa TezQA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12747423/TEZ-2627.5.modified.patch
          against master revision b046769.

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

          +1 tests included. The patch appears to include 2 new or modified test files.

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 findbugs. The patch does not introduce any new Findbugs (version 3.0.1) warnings.

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

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

          Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/927//testReport/
          Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/927//console

          This message is automatically generated.

          Show
          tezqa TezQA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12747423/TEZ-2627.5.modified.patch against master revision b046769. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 findbugs . The patch does not introduce any new Findbugs (version 3.0.1) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-TEZ-Build/927//testReport/ Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/927//console This message is automatically generated.
          Hide
          hitesh Hitesh Shah added a comment -

          Committed to master. Thanks Saikat

          Show
          hitesh Hitesh Shah added a comment - Committed to master. Thanks Saikat

            People

            • Assignee:
              saikatr Saikat
              Reporter:
              saikatr Saikat
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development