Details

    • Sub-task
    • Status: Resolved
    • Major
    • Resolution: Won't Fix
    • None
    • None
    • api, resourcemanager

    Description

      Common changes:

      • PB support changes required for Admin APIs
      • PB support for File System store (Priority Label Store)

      Attachments

        1. 0001-YARN-2896.patch
          86 kB
          Sunil G
        2. 0002-YARN-2896.patch
          97 kB
          Sunil G
        3. 0003-YARN-2896.patch
          100 kB
          Sunil G
        4. 0004-YARN-2896.patch
          99 kB
          Sunil G
        5. 0005-YARN-2896.patch
          104 kB
          Sunil G

        Activity

          sunilg Sunil G added a comment -

          Uploading an initial patch for common PB support. This patch is needed for Priority Label manager. Tests also will be added soon.

          sunilg Sunil G added a comment - Uploading an initial patch for common PB support. This patch is needed for Priority Label manager. Tests also will be added soon.
          sunilg Sunil G added a comment -

          Uploading common PB changes as a single patch

          This covers all the PB related changes in ResourceManager level. Also added tests in PBImplRecords

          I will upload a prototype patch in parent JIRA, and will upload subjira patches. As per comments, this can be made into production quality.

          sunilg Sunil G added a comment - Uploading common PB changes as a single patch This covers all the PB related changes in ResourceManager level. Also added tests in PBImplRecords I will upload a prototype patch in parent JIRA, and will upload subjira patches. As per comments, this can be made into production quality.
          sunilg Sunil G added a comment -

          Updating patch.
          Kindly check overall PB support from Server side

          sunilg Sunil G added a comment - Updating patch. Kindly check overall PB support from Server side
          hadoopqa Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12691683/0003-YARN-2896.patch
          against trunk revision d336d13.

          +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 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common.

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

          This message is automatically generated.

          hadoopqa Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12691683/0003-YARN-2896.patch against trunk revision d336d13. +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 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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 hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/6331//testReport/ Console output: https://builds.apache.org/job/PreCommit-YARN-Build/6331//console This message is automatically generated.
          sunilg Sunil G added a comment -

          Updating patch to correct test cases

          sunilg Sunil G added a comment - Updating patch to correct test cases
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12693123/0004-YARN-2896.patch
          against trunk revision 4a5c3a4.

          +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 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) 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 hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common.

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/6360//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/6360//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-api.html
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/6360//console

          This message is automatically generated.

          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12693123/0004-YARN-2896.patch against trunk revision 4a5c3a4. +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 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 2.0.3) 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 hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/6360//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/6360//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-api.html Console output: https://builds.apache.org/job/PreCommit-YARN-Build/6360//console This message is automatically generated.
          epayne Eric Payne added a comment -

          Thanks, sunilg, for working on this feature and posting this patch to support PB framework.

          I have a general question about why job priorities need labels. Why can't they just be number based? It seems like extra work to label them, pass the labels, and then interpret them.


          PriorityLabelsPerQueue.java:

            public String toString() {
              return "Max priority label: " + this.getMaxPriorityLabel() + " ,"
                  + "Default priority label: " + this.getDefaultPriorityLabel();
            }
          

          This is just a nit, but in PriorityLabelsPerQueue#toString, the space should be after the comma. Currently, this will output something like this:

              Max priority label: foo ,Default priority label: bar
          
            public int compareTo(PriorityLabelsPerQueue priorityLabelsPerQueue) {
              int defltLabelCompare = this.getDefaultPriorityLabel().compareTo(
                  priorityLabelsPerQueue.getDefaultPriorityLabel());
              if (defltLabelCompare == 0) {
                return this.getMaxPriorityLabel().compareTo(
                    priorityLabelsPerQueue.getMaxPriorityLabel());
              } else {
                return defltLabelCompare;
              }
            }
          

          PriorityLabelsPerQueue#compareTo should probably check for NULL for priorityLabelsPerQueue, this.getDefaultPriorityLabel(), and this.getMaxPriorityLabel(). If priorityLabelsPerQueue is NULL, this.getDefaultPriorityLabel() returns NULL, or this.getMaxPriorityLabel() returns NULL, compareTo will throw NPE.


          yarn_protos.proto:

          message ApplicationPriorityProto {
            optional string app_priority = 1;
          }
          

          Where is this referenced?

          epayne Eric Payne added a comment - Thanks, sunilg , for working on this feature and posting this patch to support PB framework. I have a general question about why job priorities need labels. Why can't they just be number based? It seems like extra work to label them, pass the labels, and then interpret them. PriorityLabelsPerQueue.java : public String toString() { return "Max priority label: " + this .getMaxPriorityLabel() + " ," + "Default priority label: " + this .getDefaultPriorityLabel(); } This is just a nit, but in PriorityLabelsPerQueue#toString , the space should be after the comma. Currently, this will output something like this: Max priority label: foo ,Default priority label: bar public int compareTo(PriorityLabelsPerQueue priorityLabelsPerQueue) { int defltLabelCompare = this .getDefaultPriorityLabel().compareTo( priorityLabelsPerQueue.getDefaultPriorityLabel()); if (defltLabelCompare == 0) { return this .getMaxPriorityLabel().compareTo( priorityLabelsPerQueue.getMaxPriorityLabel()); } else { return defltLabelCompare; } } PriorityLabelsPerQueue#compareTo should probably check for NULL for priorityLabelsPerQueue , this.getDefaultPriorityLabel() , and this.getMaxPriorityLabel() . If priorityLabelsPerQueue is NULL, this.getDefaultPriorityLabel() returns NULL, or this.getMaxPriorityLabel() returns NULL, compareTo will throw NPE. yarn_protos.proto : message ApplicationPriorityProto { optional string app_priority = 1; } Where is this referenced?
          leftnoteasy Wangda Tan added a comment - - edited

          I have same question as epayne, beyond readability (user can know priority of an app is "high" instead of -1), is there any must-have reason to make priority to be text?

          Priority setting in other systems like
          Linux: http://www.ibm.com/developerworks/library/l-lpic1-v3-103-6/
          Moab: https://computing.llnl.gov/tutorials/moab/#Priority
          Slurm: https://computing.llnl.gov/linux/slurm/priority_multifactor.html
          Are all using numbers, math operations can be applied on priorities, such as job_priority = queue_priority * user_priority.

          Thanks,
          Wangda

          leftnoteasy Wangda Tan added a comment - - edited I have same question as epayne , beyond readability (user can know priority of an app is "high" instead of -1), is there any must-have reason to make priority to be text? Priority setting in other systems like Linux: http://www.ibm.com/developerworks/library/l-lpic1-v3-103-6/ Moab: https://computing.llnl.gov/tutorials/moab/#Priority Slurm: https://computing.llnl.gov/linux/slurm/priority_multifactor.html Are all using numbers, math operations can be applied on priorities, such as job_priority = queue_priority * user_priority. Thanks, Wangda
          sunilg Sunil G added a comment - - edited

          Thank you epayne and wangda for the comments

          The idea of keeping Application Priority as a string is for better handling and for easiness from user perspective.
          Internally RM will have a corresponding integer mapping, and only that will be used by Schedulers. Hence as wangda mentioned, it can be operated just like an integer with user priority etc.

          A rough idea is like, user is submitting a job with priority as “High” and scheduler will be treating as an integer namely “3”.
          Priority Label Manager will act as an interface to User and Scheduler and can give the priority as string or integer accordingly.

          Now coming to the advantages, admin can operate on names or labels for priority, it will be easier. Also it can be displayed in UI very easily.
          Also admin can config the priority label as per his norms along by defining corresponding integer mapping associated with each label.
          For eg:

           
          yarn.cluster.priority-labels=low:1,medium:3,high:5
          

          Configuring ACLs based on a priority label name will be more easier.

           
          yarn.scheduler.capacity.root.queueA.High.acl=user1,user2
          

          Please share your thoughts.
          I will address the other comments from Eric and will update a patch.

          sunilg Sunil G added a comment - - edited Thank you epayne and wangda for the comments The idea of keeping Application Priority as a string is for better handling and for easiness from user perspective. Internally RM will have a corresponding integer mapping, and only that will be used by Schedulers. Hence as wangda mentioned, it can be operated just like an integer with user priority etc. A rough idea is like, user is submitting a job with priority as “High” and scheduler will be treating as an integer namely “3”. Priority Label Manager will act as an interface to User and Scheduler and can give the priority as string or integer accordingly. Now coming to the advantages, admin can operate on names or labels for priority, it will be easier. Also it can be displayed in UI very easily. Also admin can config the priority label as per his norms along by defining corresponding integer mapping associated with each label. For eg: yarn.cluster.priority-labels=low:1,medium:3,high:5 Configuring ACLs based on a priority label name will be more easier. yarn.scheduler.capacity.root.queueA.High.acl=user1,user2 Please share your thoughts. I will address the other comments from Eric and will update a patch.
          epayne Eric Payne added a comment -

          sunilg, it seems to me that labels can make things more confusing, not less, since different queues can have arbitrary names for the same concept. Also, it would eliminate the need to add an infrastructure for mapping, passing, and interpreting labels and priority numbers. YARN could always specify that priorities go from low to high, and each queue could then decide how hight to go with the priority numbers. Also, it seems to me that the following property definition could specify high priority:

          yarn.scheduler.capacity.root.queueA.5.acl=user1,user2
          
          epayne Eric Payne added a comment - sunilg , it seems to me that labels can make things more confusing, not less, since different queues can have arbitrary names for the same concept. Also, it would eliminate the need to add an infrastructure for mapping, passing, and interpreting labels and priority numbers. YARN could always specify that priorities go from low to high, and each queue could then decide how hight to go with the priority numbers. Also, it seems to me that the following property definition could specify high priority: yarn.scheduler.capacity.root.queueA.5.acl=user1,user2
          leftnoteasy Wangda Tan added a comment -

          I have similar feeling, giving a name per priority is not a scalable way, how to deal with user wants to have 100 different priorities in the cluster? If names like "priority_0".."priority_100", isn't it more clear to be a single number? People can easily compare which number is lower. If we choose number only, we can avoid a lot of complexities for PB changes and also "priority label manager" implementation.

          leftnoteasy Wangda Tan added a comment - I have similar feeling, giving a name per priority is not a scalable way, how to deal with user wants to have 100 different priorities in the cluster? If names like "priority_0".."priority_100", isn't it more clear to be a single number? People can easily compare which number is lower. If we choose number only, we can avoid a lot of complexities for PB changes and also "priority label manager" implementation.
          sunilg Sunil G added a comment - - edited

          Thank you epayne and leftnoteasy for sharing your thoughts.

          It is easier in implementation with integers and directly it can be configured and operated with. On the other hand Labels makes more readable and easier for a user to submit an app and visualize same in UI. Same for admins. But as you mentioned it comes with slighter complexity at RM to be an interface to scheduler.
          This thought of labels was one of initial idea where in design was formulated. As vinodkv also participated in design, its good to hear his thoughts too.

          Also I surely feel that we still need PriorityLabelManager, it can help us in storing the data from various options such as config files or admin commands or even REST. Its better to take such complexities out of RMAppManager. Also for HA cases, PriorityLabelManager itself can help to bring back config loaded from memory/files. Also scheduler need not have to take any load, and PrioirtyLabelManager can help in providing priority for app and its acls.

          sunilg Sunil G added a comment - - edited Thank you epayne and leftnoteasy for sharing your thoughts. It is easier in implementation with integers and directly it can be configured and operated with. On the other hand Labels makes more readable and easier for a user to submit an app and visualize same in UI. Same for admins. But as you mentioned it comes with slighter complexity at RM to be an interface to scheduler. This thought of labels was one of initial idea where in design was formulated. As vinodkv also participated in design, its good to hear his thoughts too. Also I surely feel that we still need PriorityLabelManager, it can help us in storing the data from various options such as config files or admin commands or even REST. Its better to take such complexities out of RMAppManager. Also for HA cases, PriorityLabelManager itself can help to bring back config loaded from memory/files. Also scheduler need not have to take any load, and PrioirtyLabelManager can help in providing priority for app and its acls.
          leftnoteasy Wangda Tan added a comment -

          Thanks sunilg, I agree to have a storage to simply configuration (no need to specify highest-priority for each user under each queue), all can be done via REST API or CLI. For other part, we can wait vinodkv's feedback.

          leftnoteasy Wangda Tan added a comment - Thanks sunilg , I agree to have a storage to simply configuration (no need to specify highest-priority for each user under each queue), all can be done via REST API or CLI. For other part, we can wait vinodkv 's feedback.
          epayne Eric Payne added a comment -

          sunilg, leftnoteasy, and vinodkv, can we move this discussion to YARN-1963 in order to achieve a higher visibility?

          epayne Eric Payne added a comment - sunilg , leftnoteasy , and vinodkv , can we move this discussion to YARN-1963 in order to achieve a higher visibility?
          leftnoteasy Wangda Tan added a comment -

          +1 for moving it to YARN-1963

          leftnoteasy Wangda Tan added a comment - +1 for moving it to YARN-1963
          sunilg Sunil G added a comment -

          Yes. +1.

          I will move this to parent JIRA for better visibility. Thank you Eric and Wangda for the comments.

          sunilg Sunil G added a comment - Yes. +1. I will move this to parent JIRA for better visibility. Thank you Eric and Wangda for the comments.
          sunilg Sunil G added a comment -

          Yes. +1.

          I will move this to parent JIRA for better visibility. Thank you Eric and Wangda for the comments.

          sunilg Sunil G added a comment - Yes. +1. I will move this to parent JIRA for better visibility. Thank you Eric and Wangda for the comments.
          sunilg Sunil G added a comment -

          Yes. +1.

          I will move this to parent JIRA for better visibility. Thank you Eric and Wangda for the comments.

          sunilg Sunil G added a comment - Yes. +1. I will move this to parent JIRA for better visibility. Thank you Eric and Wangda for the comments.
          sunilg Sunil G added a comment -

          Yes. +1.

          I will move this to parent JIRA for better visibility. Thank you Eric and Wangda for the comments.

          sunilg Sunil G added a comment - Yes. +1. I will move this to parent JIRA for better visibility. Thank you Eric and Wangda for the comments.
          sunilg Sunil G added a comment -

          Uploading pb related patch w/o priority label as discussed.

          sunilg Sunil G added a comment - Uploading pb related patch w/o priority label as discussed.
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12696539/0005-YARN-2896.patch
          against trunk revision 40a4157.

          +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 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) 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 hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common.

          Test results: https://builds.apache.org/job/PreCommit-YARN-Build/6512//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/6512//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-api.html
          Console output: https://builds.apache.org/job/PreCommit-YARN-Build/6512//console

          This message is automatically generated.

          hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12696539/0005-YARN-2896.patch against trunk revision 40a4157. +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 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 2.0.3) 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 hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/6512//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-YARN-Build/6512//artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-api.html Console output: https://builds.apache.org/job/PreCommit-YARN-Build/6512//console This message is automatically generated.
          sunilg Sunil G added a comment -

          Updating patch after correcting findbugs warning.

          sunilg Sunil G added a comment - Updating patch after correcting findbugs warning.
          hadoopqa Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12696768/0005-YARN-2896.patch
          against trunk revision 4e7ad4f.

          +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 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) 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 hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common.

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

          This message is automatically generated.

          hadoopqa Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12696768/0005-YARN-2896.patch against trunk revision 4e7ad4f. +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 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) 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 hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common. Test results: https://builds.apache.org/job/PreCommit-YARN-Build/6519//testReport/ Console output: https://builds.apache.org/job/PreCommit-YARN-Build/6519//console This message is automatically generated.
          sunilg Sunil G added a comment -

          This enhancement is not planned as of now.. So closing this as Wont Fix. If any discussions are happening towards this, we can reopen this.

          sunilg Sunil G added a comment - This enhancement is not planned as of now.. So closing this as Wont Fix. If any discussions are happening towards this, we can reopen this.

          People

            sunilg Sunil G
            sunilg Sunil G
            Votes:
            0 Vote for this issue
            Watchers:
            10 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: