Details

    • Type: Sub-task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: YARN-5734
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Right now there is only in-memory and leveldb based configuration store supported. Need one which supports RM HA.

      1. YARN-6840-YARN-5734.001.patch
        67 kB
        Jonathan Hung
      2. YARN-6840-YARN-5734.002.patch
        69 kB
        Jonathan Hung
      3. YARN-6840-YARN-5734.003.patch
        69 kB
        Jonathan Hung
      4. YARN-6840-YARN-5734.004.patch
        74 kB
        Jonathan Hung
      5. YARN-6840-YARN-5734.005.patch
        73 kB
        Jonathan Hung
      6. YARN-6840-YARN-5734.006.patch
        91 kB
        Jonathan Hung
      7. YARN-6840-YARN-5734.007.patch
        91 kB
        Jonathan Hung

        Issue Links

          Activity

          Hide
          templedf Daniel Templeton added a comment -

          Are you sure that's a good idea? We're already abusing ZK pretty badly with the amount of junk we cram in there as is. Zookeeper is pretty clear in the docs that it should not be used as a general purpose data store.

          Show
          templedf Daniel Templeton added a comment - Are you sure that's a good idea? We're already abusing ZK pretty badly with the amount of junk we cram in there as is. Zookeeper is pretty clear in the docs that it should not be used as a general purpose data store.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Daniel Templeton, thanks for your comments. To me, scheduler configuration updates are not general data store. Diff of config update should be small, and we can compress it before store, I expect less than 10KBs for each update for very bad cases (after compression). And frequency of config update will be much slower than application information.

          So if we continue store states of application to RMStateStore, I didn't see any issue of store configs to RMStateStore. Please let me know if you think different.

          Show
          leftnoteasy Wangda Tan added a comment - Daniel Templeton , thanks for your comments. To me, scheduler configuration updates are not general data store. Diff of config update should be small, and we can compress it before store, I expect less than 10KBs for each update for very bad cases (after compression). And frequency of config update will be much slower than application information. So if we continue store states of application to RMStateStore, I didn't see any issue of store configs to RMStateStore. Please let me know if you think different.
          Hide
          templedf Daniel Templeton added a comment -

          Just curious, how do you plan to deal with updates to the scheduler config that happen from reloading the XML file? Or are you planning to disable reloading of the file?

          Show
          templedf Daniel Templeton added a comment - Just curious, how do you plan to deal with updates to the scheduler config that happen from reloading the XML file? Or are you planning to disable reloading of the file?
          Hide
          leftnoteasy Wangda Tan added a comment -

          User has to make a choice: either using queue-refresh (reloading XML file) or using the the store-based config and use API to update configurations. YARN-6322 is tracking the change.

          Show
          leftnoteasy Wangda Tan added a comment - User has to make a choice: either using queue-refresh (reloading XML file) or using the the store-based config and use API to update configurations. YARN-6322 is tracking the change.
          Hide
          Naganarasimha Naganarasimha G R added a comment -

          Tan, Wangda,

          Diff of config update should be small, and we can compress it before store, I expect less than 10KBs for each update for very bad cases (after compression).

          So your estimation of 10kb is for each queue update? In between do we plan to store the hierarchy as zookeeper nodes or whole queue configuration under a single node. In either case, we generally tend to see 400 ~ 500 queues with our customers and expect much more to grow, would it be of any concern ?

          Show
          Naganarasimha Naganarasimha G R added a comment - Tan, Wangda , Diff of config update should be small, and we can compress it before store, I expect less than 10KBs for each update for very bad cases (after compression). So your estimation of 10kb is for each queue update? In between do we plan to store the hierarchy as zookeeper nodes or whole queue configuration under a single node. In either case, we generally tend to see 400 ~ 500 queues with our customers and expect much more to grow, would it be of any concern ?
          Hide
          leftnoteasy Wangda Tan added a comment -

          Naganarasimha G R, I think storing to a single zk node should be fine. Not sure if you could help to do an experiment: what is the size of the capacity-scheduler.xml with several hundreds queues after compression? I expect it should be less than 30kb. And as number of queue grows, compression rate should increase as well (more duplicated fields). We can also store diff of changes instead of store whole config file for every update.

          Show
          leftnoteasy Wangda Tan added a comment - Naganarasimha G R , I think storing to a single zk node should be fine. Not sure if you could help to do an experiment: what is the size of the capacity-scheduler.xml with several hundreds queues after compression? I expect it should be less than 30kb. And as number of queue grows, compression rate should increase as well (more duplicated fields). We can also store diff of changes instead of store whole config file for every update.
          Hide
          Naganarasimha Naganarasimha G R added a comment -

          Tan, Wangda,
          Can help to check that out, IIUC Based on YARN-5947 LogMutations are the objects which are stored in zk right ? Further we need to consider the case of partitions also as the whole queue hierarchy mapping gets repeated for each partition.
          But as you said there would be decrease in size initially as the contents would get repeated but if we plan to store mutations/updates only, then compression might not be of any help for mutations !

          Show
          Naganarasimha Naganarasimha G R added a comment - Tan, Wangda , Can help to check that out, IIUC Based on YARN-5947 LogMutations are the objects which are stored in zk right ? Further we need to consider the case of partitions also as the whole queue hierarchy mapping gets repeated for each partition. But as you said there would be decrease in size initially as the contents would get repeated but if we plan to store mutations/updates only, then compression might not be of any help for mutations !
          Hide
          leftnoteasy Wangda Tan added a comment -

          Naganarasimha G R, thanks, I think compress could still help here: for serialized stream of LogMutation, we can compress it before send to backend store.

          Show
          leftnoteasy Wangda Tan added a comment - Naganarasimha G R , thanks, I think compress could still help here: for serialized stream of LogMutation, we can compress it before send to backend store.
          Hide
          zhz Zhe Zhang added a comment -

          Could this task leverage logic from HDFS-10631 and YARN-6900?

          Show
          zhz Zhe Zhang added a comment - Could this task leverage logic from HDFS-10631 and YARN-6900 ?
          Hide
          leftnoteasy Wangda Tan added a comment -

          Zhe Zhang,

          I'm not sure if we can leverage them, it will be good if we can have same basic ZK implementation for all related tasks. However, pulling common code out may potentially destabilize existing features. I don't have strong opinion on this, if existing feature owner is supportive for this, I will be good .

          + Íñigo Goiri/Subru Krishnan. Do you think we can pull common ZK implementation out and make it in this feature as well?

          Show
          leftnoteasy Wangda Tan added a comment - Zhe Zhang , I'm not sure if we can leverage them, it will be good if we can have same basic ZK implementation for all related tasks. However, pulling common code out may potentially destabilize existing features. I don't have strong opinion on this, if existing feature owner is supportive for this, I will be good . + Íñigo Goiri / Subru Krishnan . Do you think we can pull common ZK implementation out and make it in this feature as well?
          Hide
          elgoiri Íñigo Goiri added a comment -

          Tan, Wangda, we are already using curator for HDFS-10631 and YARN-6900.
          Curator already wraps some of the common functionality.
          There might be some functions that can be made into a utility but not sure if there is too much value.

          Show
          elgoiri Íñigo Goiri added a comment - Tan, Wangda , we are already using curator for HDFS-10631 and YARN-6900 . Curator already wraps some of the common functionality. There might be some functions that can be made into a utility but not sure if there is too much value.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Íñigo Goiri, thanks for your comments, actually I'm looking for some common functionalities on top of Curator. If there's no much commonality between these features, I'm fine to implement this feature by using Curator separately.

          Show
          leftnoteasy Wangda Tan added a comment - Íñigo Goiri , thanks for your comments, actually I'm looking for some common functionalities on top of Curator. If there's no much commonality between these features, I'm fine to implement this feature by using Curator separately.
          Hide
          subru Subru Krishnan added a comment -

          Zhe Zhang/Wangda Tan, I am reviewing YARN-6900 and I observed parts of the curator code that are dealing with ZK interactions are being repeated so I have requested Íñigo Goiri to refactor them to a common util class here.

          Show
          subru Subru Krishnan added a comment - Zhe Zhang / Wangda Tan , I am reviewing YARN-6900 and I observed parts of the curator code that are dealing with ZK interactions are being repeated so I have requested Íñigo Goiri to refactor them to a common util class here .
          Hide
          drankye Kai Zheng added a comment -

          Sharing some codes in Hadoop common sounds good. Or further, not sure if it's good to have a generic Hadoop StatesStore (ZK maybe as the default back end) across both HDFS and YARN. As I don't think there is very heavy usage of the store (so can't share) so I guess the same store could be used to simplify the deployment. We might evolve towards that.

          Show
          drankye Kai Zheng added a comment - Sharing some codes in Hadoop common sounds good. Or further, not sure if it's good to have a generic Hadoop StatesStore (ZK maybe as the default back end) across both HDFS and YARN. As I don't think there is very heavy usage of the store (so can't share) so I guess the same store could be used to simplify the deployment. We might evolve towards that.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Jonathan Hung, HADOOP-14741 is close to be committed, could you check if you can use the common framework added by HADOOP-14741?

          Show
          leftnoteasy Wangda Tan added a comment - Jonathan Hung , HADOOP-14741 is close to be committed, could you check if you can use the common framework added by HADOOP-14741 ?
          Hide
          subru Subru Krishnan added a comment -

          Jonathan Huang/Wangda Tan, I committed HADOOP-14741 as it's blocking YARN-6900. But do take a look and let us know if you have any feedback, we can always fix in a follow-up jira.

          FYI the rebased version of YARN-6900 post HADOOP-14741 commit has only Federation specific ZK code, so already looking good .

          Show
          subru Subru Krishnan added a comment - Jonathan Huang / Wangda Tan , I committed HADOOP-14741 as it's blocking YARN-6900 . But do take a look and let us know if you have any feedback, we can always fix in a follow-up jira. FYI the rebased version of YARN-6900 post HADOOP-14741 commit has only Federation specific ZK code, so already looking good .
          Hide
          jhung Jonathan Hung added a comment -

          Great, thanks Tan, Wangda/Subru Krishnan, will base this ticket on HADOOP-14741.

          Show
          jhung Jonathan Hung added a comment - Great, thanks Tan, Wangda / Subru Krishnan , will base this ticket on HADOOP-14741 .
          Hide
          jhung Jonathan Hung added a comment -

          Hi Wangda Tan and Xuan Gong, posted a zookeeper store implementation 001 version.

          Show
          jhung Jonathan Hung added a comment - Hi Wangda Tan and Xuan Gong , posted a zookeeper store implementation 001 version.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
                YARN-5734 Compile Tests
          0 mvndep 2m 48s Maven dependency ordering for branch
          +1 mvninstall 15m 18s YARN-5734 passed
          +1 compile 9m 27s YARN-5734 passed
          +1 checkstyle 0m 54s YARN-5734 passed
          +1 mvnsite 1m 17s YARN-5734 passed
          +1 findbugs 2m 11s YARN-5734 passed
          +1 javadoc 0m 56s YARN-5734 passed
                Patch Compile Tests
          0 mvndep 0m 11s Maven dependency ordering for patch
          +1 mvninstall 0m 54s the patch passed
          +1 compile 5m 41s the patch passed
          +1 javac 5m 41s the patch passed
          -0 checkstyle 0m 53s hadoop-yarn-project/hadoop-yarn: The patch generated 36 new + 328 unchanged - 3 fixed = 364 total (was 331)
          +1 mvnsite 1m 4s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 findbugs 1m 14s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
          -1 javadoc 0m 27s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 1 new + 347 unchanged - 0 fixed = 348 total (was 347)
                Other Tests
          -1 unit 0m 28s hadoop-yarn-api in the patch failed.
          -1 unit 45m 6s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 27s The patch does not generate ASF License warnings.
          99m 36s



          Reason Tests
          FindBugs module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
            Found reliance on default encoding in org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.conf.ZKConfigurationStore.confirmMutation(long, boolean):in org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.conf.ZKConfigurationStore.confirmMutation(long, boolean): String.getBytes() At ZKConfigurationStore.java:[line 190]
          Failed junit tests hadoop.yarn.conf.TestYarnConfigurationFields
            hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6840
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12883778/YARN-6840-YARN-5734.001.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 60a1e360e0a7 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-5734 / 98c4a52
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17131/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          findbugs https://builds.apache.org/job/PreCommit-YARN-Build/17131/artifact/patchprocess/new-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.html
          javadoc https://builds.apache.org/job/PreCommit-YARN-Build/17131/artifact/patchprocess/diff-javadoc-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/17131/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-api.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/17131/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17131/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/17131/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.       YARN-5734 Compile Tests 0 mvndep 2m 48s Maven dependency ordering for branch +1 mvninstall 15m 18s YARN-5734 passed +1 compile 9m 27s YARN-5734 passed +1 checkstyle 0m 54s YARN-5734 passed +1 mvnsite 1m 17s YARN-5734 passed +1 findbugs 2m 11s YARN-5734 passed +1 javadoc 0m 56s YARN-5734 passed       Patch Compile Tests 0 mvndep 0m 11s Maven dependency ordering for patch +1 mvninstall 0m 54s the patch passed +1 compile 5m 41s the patch passed +1 javac 5m 41s the patch passed -0 checkstyle 0m 53s hadoop-yarn-project/hadoop-yarn: The patch generated 36 new + 328 unchanged - 3 fixed = 364 total (was 331) +1 mvnsite 1m 4s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 findbugs 1m 14s hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) -1 javadoc 0m 27s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 1 new + 347 unchanged - 0 fixed = 348 total (was 347)       Other Tests -1 unit 0m 28s hadoop-yarn-api in the patch failed. -1 unit 45m 6s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 27s The patch does not generate ASF License warnings. 99m 36s Reason Tests FindBugs module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager   Found reliance on default encoding in org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.conf.ZKConfigurationStore.confirmMutation(long, boolean):in org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.conf.ZKConfigurationStore.confirmMutation(long, boolean): String.getBytes() At ZKConfigurationStore.java: [line 190] Failed junit tests hadoop.yarn.conf.TestYarnConfigurationFields   hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6840 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12883778/YARN-6840-YARN-5734.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 60a1e360e0a7 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-5734 / 98c4a52 Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17131/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt findbugs https://builds.apache.org/job/PreCommit-YARN-Build/17131/artifact/patchprocess/new-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.html javadoc https://builds.apache.org/job/PreCommit-YARN-Build/17131/artifact/patchprocess/diff-javadoc-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/17131/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-api.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/17131/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17131/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/17131/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jhung Jonathan Hung added a comment -

          002 fixes checkstyle, findbugs, javadoc, and related unit tests

          Show
          jhung Jonathan Hung added a comment - 002 fixes checkstyle, findbugs, javadoc, and related unit tests
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
                YARN-5734 Compile Tests
          0 mvndep 2m 49s Maven dependency ordering for branch
          +1 mvninstall 17m 42s YARN-5734 passed
          +1 compile 10m 44s YARN-5734 passed
          +1 checkstyle 1m 5s YARN-5734 passed
          +1 mvnsite 2m 6s YARN-5734 passed
          +1 findbugs 3m 53s YARN-5734 passed
          +1 javadoc 1m 42s YARN-5734 passed
                Patch Compile Tests
          0 mvndep 0m 11s Maven dependency ordering for patch
          +1 mvninstall 1m 47s the patch passed
          +1 compile 6m 17s the patch passed
          +1 javac 6m 17s the patch passed
          -0 checkstyle 0m 58s hadoop-yarn-project/hadoop-yarn: The patch generated 4 new + 328 unchanged - 2 fixed = 332 total (was 330)
          +1 mvnsite 1m 53s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 4m 9s the patch passed
          +1 javadoc 1m 43s the patch passed
                Other Tests
          +1 unit 0m 36s hadoop-yarn-api in the patch passed.
          +1 unit 2m 43s hadoop-yarn-common in the patch passed.
          -1 unit 46m 10s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 34s The patch does not generate ASF License warnings.
          115m 42s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6840
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12883810/YARN-6840-YARN-5734.002.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 235c8cf7ac9c 3.13.0-123-generic #172-Ubuntu SMP Mon Jun 26 18:04:35 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-5734 / 98c4a52
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17137/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/17137/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17137/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/17137/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.       YARN-5734 Compile Tests 0 mvndep 2m 49s Maven dependency ordering for branch +1 mvninstall 17m 42s YARN-5734 passed +1 compile 10m 44s YARN-5734 passed +1 checkstyle 1m 5s YARN-5734 passed +1 mvnsite 2m 6s YARN-5734 passed +1 findbugs 3m 53s YARN-5734 passed +1 javadoc 1m 42s YARN-5734 passed       Patch Compile Tests 0 mvndep 0m 11s Maven dependency ordering for patch +1 mvninstall 1m 47s the patch passed +1 compile 6m 17s the patch passed +1 javac 6m 17s the patch passed -0 checkstyle 0m 58s hadoop-yarn-project/hadoop-yarn: The patch generated 4 new + 328 unchanged - 2 fixed = 332 total (was 330) +1 mvnsite 1m 53s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 4m 9s the patch passed +1 javadoc 1m 43s the patch passed       Other Tests +1 unit 0m 36s hadoop-yarn-api in the patch passed. +1 unit 2m 43s hadoop-yarn-common in the patch passed. -1 unit 46m 10s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 34s The patch does not generate ASF License warnings. 115m 42s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6840 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12883810/YARN-6840-YARN-5734.002.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 235c8cf7ac9c 3.13.0-123-generic #172-Ubuntu SMP Mon Jun 26 18:04:35 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-5734 / 98c4a52 Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17137/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/17137/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17137/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/17137/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          xgong Xuan Gong added a comment -

          Thanks for the fix.
          Overall looks fine.

          In logMutation(LogMutation logMutation), should we use zkManager.safeSetData instead of zkManager.setData to store the log ?

          Show
          xgong Xuan Gong added a comment - Thanks for the fix. Overall looks fine. In logMutation(LogMutation logMutation), should we use zkManager.safeSetData instead of zkManager.setData to store the log ?
          Hide
          jhung Jonathan Hung added a comment -

          Thanks Xuan Gong - uploaded 003 which changes this.

          Show
          jhung Jonathan Hung added a comment - Thanks Xuan Gong - uploaded 003 which changes this.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 3m 30s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
                YARN-5734 Compile Tests
          0 mvndep 0m 54s Maven dependency ordering for branch
          +1 mvninstall 15m 0s YARN-5734 passed
          +1 compile 10m 10s YARN-5734 passed
          +1 checkstyle 1m 1s YARN-5734 passed
          +1 mvnsite 1m 56s YARN-5734 passed
          +1 findbugs 3m 44s YARN-5734 passed
          +1 javadoc 1m 45s YARN-5734 passed
                Patch Compile Tests
          0 mvndep 0m 10s Maven dependency ordering for patch
          -1 mvninstall 0m 13s hadoop-yarn-server-resourcemanager in the patch failed.
          -1 compile 2m 13s hadoop-yarn in the patch failed.
          -1 javac 2m 13s hadoop-yarn in the patch failed.
          -0 checkstyle 0m 42s hadoop-yarn-project/hadoop-yarn: The patch generated 1 new + 208 unchanged - 122 fixed = 209 total (was 330)
          -1 mvnsite 0m 16s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          -1 findbugs 0m 15s hadoop-yarn-server-resourcemanager in the patch failed.
          -1 javadoc 0m 12s hadoop-yarn-server-resourcemanager in the patch failed.
                Other Tests
          +1 unit 0m 28s hadoop-yarn-api in the patch passed.
          +1 unit 2m 27s hadoop-yarn-common in the patch passed.
          -1 unit 0m 15s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 16s The patch does not generate ASF License warnings.
          58m 48s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6840
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12885958/YARN-6840-YARN-5734.003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 814b0e56ca82 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-5734 / 98c4a52
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          mvninstall https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/patch-mvninstall-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          compile https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn.txt
          javac https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn.txt
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          mvnsite https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/patch-mvnsite-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          findbugs https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/patch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          javadoc https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/patch-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17345/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/17345/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 3m 30s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.       YARN-5734 Compile Tests 0 mvndep 0m 54s Maven dependency ordering for branch +1 mvninstall 15m 0s YARN-5734 passed +1 compile 10m 10s YARN-5734 passed +1 checkstyle 1m 1s YARN-5734 passed +1 mvnsite 1m 56s YARN-5734 passed +1 findbugs 3m 44s YARN-5734 passed +1 javadoc 1m 45s YARN-5734 passed       Patch Compile Tests 0 mvndep 0m 10s Maven dependency ordering for patch -1 mvninstall 0m 13s hadoop-yarn-server-resourcemanager in the patch failed. -1 compile 2m 13s hadoop-yarn in the patch failed. -1 javac 2m 13s hadoop-yarn in the patch failed. -0 checkstyle 0m 42s hadoop-yarn-project/hadoop-yarn: The patch generated 1 new + 208 unchanged - 122 fixed = 209 total (was 330) -1 mvnsite 0m 16s hadoop-yarn-server-resourcemanager in the patch failed. +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. -1 findbugs 0m 15s hadoop-yarn-server-resourcemanager in the patch failed. -1 javadoc 0m 12s hadoop-yarn-server-resourcemanager in the patch failed.       Other Tests +1 unit 0m 28s hadoop-yarn-api in the patch passed. +1 unit 2m 27s hadoop-yarn-common in the patch passed. -1 unit 0m 15s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 58m 48s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6840 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12885958/YARN-6840-YARN-5734.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 814b0e56ca82 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-5734 / 98c4a52 Default Java 1.8.0_144 findbugs v3.1.0-RC1 mvninstall https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/patch-mvninstall-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt compile https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn.txt javac https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn.txt checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt mvnsite https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/patch-mvnsite-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt findbugs https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/patch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt javadoc https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/patch-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/17345/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17345/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/17345/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 21s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
                YARN-5734 Compile Tests
          0 mvndep 2m 51s Maven dependency ordering for branch
          +1 mvninstall 16m 8s YARN-5734 passed
          +1 compile 10m 24s YARN-5734 passed
          +1 checkstyle 1m 4s YARN-5734 passed
          +1 mvnsite 2m 2s YARN-5734 passed
          +1 findbugs 3m 46s YARN-5734 passed
          +1 javadoc 1m 45s YARN-5734 passed
                Patch Compile Tests
          0 mvndep 0m 11s Maven dependency ordering for patch
          +1 mvninstall 1m 40s the patch passed
          +1 compile 6m 17s the patch passed
          +1 javac 6m 17s the patch passed
          -0 checkstyle 1m 4s hadoop-yarn-project/hadoop-yarn: The patch generated 4 new + 328 unchanged - 2 fixed = 332 total (was 330)
          +1 mvnsite 2m 5s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 4m 6s the patch passed
          +1 javadoc 1m 37s the patch passed
                Other Tests
          +1 unit 0m 37s hadoop-yarn-api in the patch passed.
          +1 unit 2m 40s hadoop-yarn-common in the patch passed.
          -1 unit 45m 13s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 29s The patch does not generate ASF License warnings.
          112m 45s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6840
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12885976/YARN-6840-YARN-5734.003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 04cc49068525 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-5734 / 98c4a52
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17351/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/17351/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17351/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/17351/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 21s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.       YARN-5734 Compile Tests 0 mvndep 2m 51s Maven dependency ordering for branch +1 mvninstall 16m 8s YARN-5734 passed +1 compile 10m 24s YARN-5734 passed +1 checkstyle 1m 4s YARN-5734 passed +1 mvnsite 2m 2s YARN-5734 passed +1 findbugs 3m 46s YARN-5734 passed +1 javadoc 1m 45s YARN-5734 passed       Patch Compile Tests 0 mvndep 0m 11s Maven dependency ordering for patch +1 mvninstall 1m 40s the patch passed +1 compile 6m 17s the patch passed +1 javac 6m 17s the patch passed -0 checkstyle 1m 4s hadoop-yarn-project/hadoop-yarn: The patch generated 4 new + 328 unchanged - 2 fixed = 332 total (was 330) +1 mvnsite 2m 5s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 4m 6s the patch passed +1 javadoc 1m 37s the patch passed       Other Tests +1 unit 0m 37s hadoop-yarn-api in the patch passed. +1 unit 2m 40s hadoop-yarn-common in the patch passed. -1 unit 45m 13s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 29s The patch does not generate ASF License warnings. 112m 45s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6840 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12885976/YARN-6840-YARN-5734.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 04cc49068525 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-5734 / 98c4a52 Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17351/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/17351/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17351/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/17351/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Thanks Jonathan Hung for working on this patch, just reviewed overall workflows of the patch:

          Thanks Jonathan,

          Apologize for the late review, some questions/comments:

          In AdminService:

          • Move following code:
                if (isActiveTransition && isSchedulerMutable()) {
                  try {
                    ((MutableConfScheduler) rm.getRMContext().getScheduler())
                        .refreshConfiguration();
                  } catch (Exception e) {
                    throw new IOException("Failed to refresh configuration:", e);
                  }
                }
            

            To refreshAll, and parameter isActiveTransition and private void refreshQueues can be removed.

          • Is it a better idea to move public void refreshQueues() logic to scheduler#reinitialize(...). It's not a good idea to invoke AdminService#refreshQueues from MutableCSConfigurationProvider. (Please make sure that don't acquire any lock of scheduler while invoking ReservationSystem.reinitialize()).

          In MutableCSConfigurationProvider:

          • It's better to remove:
              @VisibleForTesting
              YarnConfigurationStore getConfStore();
            

            From the base class. Unit test can call implementation's methods.

          In MutableConfScheduler:

          • Similar to above, it's better to remove:
              @VisibleForTesting
              MutableConfigurationProvider getMutableConfProvider();
            

            From the base class.

          • refreshConfiguration -> reloadConfigurationFromStore

          In ResourceManager:

          • createAndStartZKManager can be merged to rm#getZKManager(), maybe it's better to rename it to rm#getAndInitializeZKManager(). Even if there's no race condition issue since this is only called in RM initialization. It's better to add synchronize to the method.
          • Getter API of ResourceManager should be exposed by RMContext. We should never use ref to ResourceManager directly.

          In YarnConfigurationStore:

          • setResourceManager can be removed and you can pass RMContext ref to initialize.
          • What happens if Configuration schedConf passed to a already initialized store?
          • Could you add Javadocs to following methods:
              protected abstract Version getConfStoreVersion() throws Exception;
            
              protected abstract void storeVersion() throws Exception;
            
              protected abstract Version getCurrentVersion();
            

          I will leave ZK-related implementation reviews to Xuan Gong.

          Show
          leftnoteasy Wangda Tan added a comment - Thanks Jonathan Hung for working on this patch, just reviewed overall workflows of the patch: Thanks Jonathan, Apologize for the late review, some questions/comments: In AdminService: Move following code: if (isActiveTransition && isSchedulerMutable()) { try { ((MutableConfScheduler) rm.getRMContext().getScheduler()) .refreshConfiguration(); } catch (Exception e) { throw new IOException( "Failed to refresh configuration:" , e); } } To refreshAll, and parameter isActiveTransition and private void refreshQueues can be removed. Is it a better idea to move public void refreshQueues() logic to scheduler#reinitialize(...). It's not a good idea to invoke AdminService#refreshQueues from MutableCSConfigurationProvider. (Please make sure that don't acquire any lock of scheduler while invoking ReservationSystem.reinitialize()). In MutableCSConfigurationProvider: It's better to remove: @VisibleForTesting YarnConfigurationStore getConfStore(); From the base class. Unit test can call implementation's methods. In MutableConfScheduler: Similar to above, it's better to remove: @VisibleForTesting MutableConfigurationProvider getMutableConfProvider(); From the base class. refreshConfiguration -> reloadConfigurationFromStore In ResourceManager: createAndStartZKManager can be merged to rm#getZKManager(), maybe it's better to rename it to rm#getAndInitializeZKManager(). Even if there's no race condition issue since this is only called in RM initialization. It's better to add synchronize to the method. Getter API of ResourceManager should be exposed by RMContext. We should never use ref to ResourceManager directly. In YarnConfigurationStore: setResourceManager can be removed and you can pass RMContext ref to initialize . What happens if Configuration schedConf passed to a already initialized store? Could you add Javadocs to following methods: protected abstract Version getConfStoreVersion() throws Exception; protected abstract void storeVersion() throws Exception; protected abstract Version getCurrentVersion(); I will leave ZK-related implementation reviews to Xuan Gong .
          Hide
          jhung Jonathan Hung added a comment - - edited

          Thanks Wangda Tan for the review. Attached 004 addressing most of these comments. Also added a couple of sleeps in TestZKConfigurationStore due to a race condition to fix some test failures.

          Move following code: To refreshAll, and parameter isActiveTransition and private void refreshQueues can be removed.

          Done

          Is it a better idea to move public void refreshQueues() logic to scheduler#reinitialize(...). It's not a good idea to invoke AdminService#refreshQueues from MutableCSConfigurationProvider.

          Not sure about this, then we are doing the reservation system reinitialization inside scheduler, so every time scheduler#reinitialize is called, the reservation system is also initialized, not sure if this is the desired behavior. Also we would need to duplicate the reservation system reinitialization for all schedulers, or make ResourceScheduler an abstract class and add it there.
          Unless you meant duplicating the reservation system reinitialization logic inside MutableCSConfigurationProvider#mutateConfiguration? I think this makes more sense, but then we have duplicate code between this and AdminService.

          In MutableCSConfigurationProvider: It's better to remove:...

          Done

          In MutableConfScheduler: Similar to above, it's better to remove:...

          Done

          refreshConfiguration -> reloadConfigurationFromStore

          Done

          createAndStartZKManager can be merged to rm#getZKManager()

          Renamed to getAndStartZKManager

          Getter API of ResourceManager should be exposed by RMContext. We should never use ref to ResourceManager directly.

          Done for ZKConfigurationStore, I left the references in ZKRMStateStore since this class has other direct references to rm object. We can handle this in a separate ticket if you'd like.

          setResourceManager can be removed and you can pass RMContext ref to initialize.

          Done

          What happens if Configuration schedConf passed to a already initialized store?

          For leveldb and zk, it will ignore it and use the scheduler configuration persisted in the store.

          Could you add Javadocs to following methods:

          Done

          Show
          jhung Jonathan Hung added a comment - - edited Thanks Wangda Tan for the review. Attached 004 addressing most of these comments. Also added a couple of sleeps in TestZKConfigurationStore due to a race condition to fix some test failures. Move following code: To refreshAll, and parameter isActiveTransition and private void refreshQueues can be removed. Done Is it a better idea to move public void refreshQueues() logic to scheduler#reinitialize(...). It's not a good idea to invoke AdminService#refreshQueues from MutableCSConfigurationProvider. Not sure about this, then we are doing the reservation system reinitialization inside scheduler, so every time scheduler#reinitialize is called, the reservation system is also initialized, not sure if this is the desired behavior. Also we would need to duplicate the reservation system reinitialization for all schedulers, or make ResourceScheduler an abstract class and add it there. Unless you meant duplicating the reservation system reinitialization logic inside MutableCSConfigurationProvider#mutateConfiguration? I think this makes more sense, but then we have duplicate code between this and AdminService. In MutableCSConfigurationProvider: It's better to remove:... Done In MutableConfScheduler: Similar to above, it's better to remove:... Done refreshConfiguration -> reloadConfigurationFromStore Done createAndStartZKManager can be merged to rm#getZKManager() Renamed to getAndStartZKManager Getter API of ResourceManager should be exposed by RMContext. We should never use ref to ResourceManager directly. Done for ZKConfigurationStore, I left the references in ZKRMStateStore since this class has other direct references to rm object. We can handle this in a separate ticket if you'd like. setResourceManager can be removed and you can pass RMContext ref to initialize. Done What happens if Configuration schedConf passed to a already initialized store? For leveldb and zk, it will ignore it and use the scheduler configuration persisted in the store. Could you add Javadocs to following methods: Done
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 20m 1s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
                YARN-5734 Compile Tests
          0 mvndep 2m 44s Maven dependency ordering for branch
          +1 mvninstall 15m 56s YARN-5734 passed
          +1 compile 10m 9s YARN-5734 passed
          +1 checkstyle 0m 57s YARN-5734 passed
          +1 mvnsite 1m 51s YARN-5734 passed
          +1 findbugs 3m 45s YARN-5734 passed
          +1 javadoc 1m 31s YARN-5734 passed
                Patch Compile Tests
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 38s the patch passed
          +1 compile 6m 27s the patch passed
          +1 javac 6m 27s the patch passed
          -0 checkstyle 0m 56s hadoop-yarn-project/hadoop-yarn: The patch generated 10 new + 364 unchanged - 3 fixed = 374 total (was 367)
          +1 mvnsite 1m 57s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 4m 23s the patch passed
          +1 javadoc 1m 33s the patch passed
                Other Tests
          +1 unit 0m 35s hadoop-yarn-api in the patch passed.
          +1 unit 2m 50s hadoop-yarn-common in the patch passed.
          -1 unit 46m 33s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 24s The patch does not generate ASF License warnings.
          133m 30s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6840
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12886995/YARN-6840-YARN-5734.004.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux dcfc2a7e719f 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-5734 / 98c4a52
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17447/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/17447/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17447/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/17447/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 20m 1s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.       YARN-5734 Compile Tests 0 mvndep 2m 44s Maven dependency ordering for branch +1 mvninstall 15m 56s YARN-5734 passed +1 compile 10m 9s YARN-5734 passed +1 checkstyle 0m 57s YARN-5734 passed +1 mvnsite 1m 51s YARN-5734 passed +1 findbugs 3m 45s YARN-5734 passed +1 javadoc 1m 31s YARN-5734 passed       Patch Compile Tests 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 38s the patch passed +1 compile 6m 27s the patch passed +1 javac 6m 27s the patch passed -0 checkstyle 0m 56s hadoop-yarn-project/hadoop-yarn: The patch generated 10 new + 364 unchanged - 3 fixed = 374 total (was 367) +1 mvnsite 1m 57s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 4m 23s the patch passed +1 javadoc 1m 33s the patch passed       Other Tests +1 unit 0m 35s hadoop-yarn-api in the patch passed. +1 unit 2m 50s hadoop-yarn-common in the patch passed. -1 unit 46m 33s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 24s The patch does not generate ASF License warnings. 133m 30s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6840 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12886995/YARN-6840-YARN-5734.004.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux dcfc2a7e719f 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-5734 / 98c4a52 Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17447/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/17447/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17447/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/17447/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jhung Jonathan Hung added a comment -

          Fix some checkstyle issues (005)

          Show
          jhung Jonathan Hung added a comment - Fix some checkstyle issues (005)
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 19m 11s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
                YARN-5734 Compile Tests
          0 mvndep 4m 0s Maven dependency ordering for branch
          +1 mvninstall 14m 47s YARN-5734 passed
          +1 compile 8m 57s YARN-5734 passed
          +1 checkstyle 1m 1s YARN-5734 passed
          +1 mvnsite 1m 58s YARN-5734 passed
          +1 findbugs 3m 25s YARN-5734 passed
          +1 javadoc 1m 40s YARN-5734 passed
                Patch Compile Tests
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 24s the patch passed
          +1 compile 5m 28s the patch passed
          +1 javac 5m 28s the patch passed
          -0 checkstyle 1m 0s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 365 unchanged - 3 fixed = 370 total (was 368)
          +1 mvnsite 1m 54s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 2s The patch has no ill-formed XML file.
          +1 findbugs 3m 41s the patch passed
          +1 javadoc 1m 37s the patch passed
                Other Tests
          +1 unit 0m 34s hadoop-yarn-api in the patch passed.
          +1 unit 2m 38s hadoop-yarn-common in the patch passed.
          -1 unit 46m 7s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 30s The patch does not generate ASF License warnings.
          129m 59s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation
            hadoop.yarn.server.resourcemanager.scheduler.fair.TestFSAppStarvation



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6840
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12887019/YARN-6840-YARN-5734.005.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux ce70e7d4dca3 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-5734 / 98c4a52
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17452/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/17452/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17452/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/17452/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 19m 11s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.       YARN-5734 Compile Tests 0 mvndep 4m 0s Maven dependency ordering for branch +1 mvninstall 14m 47s YARN-5734 passed +1 compile 8m 57s YARN-5734 passed +1 checkstyle 1m 1s YARN-5734 passed +1 mvnsite 1m 58s YARN-5734 passed +1 findbugs 3m 25s YARN-5734 passed +1 javadoc 1m 40s YARN-5734 passed       Patch Compile Tests 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 24s the patch passed +1 compile 5m 28s the patch passed +1 javac 5m 28s the patch passed -0 checkstyle 1m 0s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 365 unchanged - 3 fixed = 370 total (was 368) +1 mvnsite 1m 54s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 findbugs 3m 41s the patch passed +1 javadoc 1m 37s the patch passed       Other Tests +1 unit 0m 34s hadoop-yarn-api in the patch passed. +1 unit 2m 38s hadoop-yarn-common in the patch passed. -1 unit 46m 7s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 30s The patch does not generate ASF License warnings. 129m 59s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation   hadoop.yarn.server.resourcemanager.scheduler.fair.TestFSAppStarvation Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6840 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12887019/YARN-6840-YARN-5734.005.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux ce70e7d4dca3 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-5734 / 98c4a52 Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17452/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/17452/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17452/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/17452/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Thanks Jonathan Hung for updating the patch,

          For leveldb and zk, it will ignore it and use the scheduler configuration persisted in the store.

          Then I suggest to add this as a Javadoc to the base class's method, this should be respected by all future implementations. Otherwise behavior will be changed when different store is configured.

          Not sure about this, then we are doing the reservation system reinitialization inside scheduler, so every time scheduler#reinitialize is called, the reservation system is also initialized, not sure if this is the desired behavior. Also we would need to duplicate the reservation system reinitialization for all schedulers, or make ResourceScheduler an abstract class and add it there. ...

          I just checked the code, probably I should use a different way to describe the problem:
          There're two different code path to refresh scheduler config:
          Path #1 (When mutation disabled) : Client -> AdminService -> Scheduler/ReservationSystem#reinitialize
          Path #2 (When mutation enabled) : Client -> RMWebService -> Scheduler -> ConfProvider (do log persistent) -> AdminService -> Scheduler/ReservationSystem#reinitialize -> ConfProvider (confirm or discard mutation).

          Please note that in the different code path, ordering of scheduler and AdminService is inverted, this is confusing and could possibly cause deadlock, etc.

          Here's my proposal:
          1) Change MutableConfigurationProvider#mutateConfiguration to log-scheduler-config-mutation, it will do following things:
          a. Merge mutations to existing configs.
          b. Call confStore.logMutation to persistent it.

          2) Add two new method to MutableConfigurationProvider
          a. Confirm last mutation - confirm last logged mutation. (Just call YarnConfigurationStore#confirmMutation(valid = true))
          b. Discard last mutation - discard last logged mutation. (Just call YarnConfigurationStore#confirmMutation(valid = false))
          And is it possible to remove id field in the confirmMutation method? Should we allow at most one pending mutation?

          One we have above, the call path#2 becomes:
          (1) Client -> RMWebService#updateSchedulerConfiguration -> MutableConfigurationProvider#log-scheduler-config-mutation
          (2) ... RMWebService#updateSchedulerConfiguration -> AdminService#refreshQueues -> Scheduler/ReservationSystem#reinitialize.

          If reinitialize succeeded:
          (3) ... RMWebService#updateSchedulerConfiguration -> MutableConfigurationProvider#confirmLastChange

          If reinitialize failed:
          (4) RMWebService#updateSchedulerConfiguration -> MutableConfigurationProvider#discardLastChange

          Show
          leftnoteasy Wangda Tan added a comment - Thanks Jonathan Hung for updating the patch, For leveldb and zk, it will ignore it and use the scheduler configuration persisted in the store. Then I suggest to add this as a Javadoc to the base class's method, this should be respected by all future implementations. Otherwise behavior will be changed when different store is configured. Not sure about this, then we are doing the reservation system reinitialization inside scheduler, so every time scheduler#reinitialize is called, the reservation system is also initialized, not sure if this is the desired behavior. Also we would need to duplicate the reservation system reinitialization for all schedulers, or make ResourceScheduler an abstract class and add it there. ... I just checked the code, probably I should use a different way to describe the problem: There're two different code path to refresh scheduler config: Path #1 (When mutation disabled) : Client -> AdminService -> Scheduler/ReservationSystem#reinitialize Path #2 (When mutation enabled) : Client -> RMWebService -> Scheduler -> ConfProvider (do log persistent) -> AdminService -> Scheduler/ReservationSystem#reinitialize -> ConfProvider (confirm or discard mutation). Please note that in the different code path, ordering of scheduler and AdminService is inverted, this is confusing and could possibly cause deadlock, etc. Here's my proposal: 1) Change MutableConfigurationProvider#mutateConfiguration to log-scheduler-config-mutation, it will do following things: a. Merge mutations to existing configs. b. Call confStore.logMutation to persistent it. 2) Add two new method to MutableConfigurationProvider a. Confirm last mutation - confirm last logged mutation. (Just call YarnConfigurationStore#confirmMutation(valid = true)) b. Discard last mutation - discard last logged mutation. (Just call YarnConfigurationStore#confirmMutation(valid = false)) And is it possible to remove id field in the confirmMutation method? Should we allow at most one pending mutation? One we have above, the call path#2 becomes: (1) Client -> RMWebService#updateSchedulerConfiguration -> MutableConfigurationProvider#log-scheduler-config-mutation (2) ... RMWebService#updateSchedulerConfiguration -> AdminService#refreshQueues -> Scheduler/ReservationSystem#reinitialize. If reinitialize succeeded: (3) ... RMWebService#updateSchedulerConfiguration -> MutableConfigurationProvider#confirmLastChange If reinitialize failed: (4) RMWebService#updateSchedulerConfiguration -> MutableConfigurationProvider#discardLastChange
          Hide
          leftnoteasy Wangda Tan added a comment -

          Synced with Jonathan Hung offline, in additional to above suggestions, other changes:
          1) Avoid call CapacityScheduler methods from provider.
          2) Discard pending mutation during recovery. (Make sure that AdminService#refreshAll won't include the pending mutation too).
          3) MutableScheduler#updateConfiguration can be removed. Instead we need an API to get confProvider.

          Show
          leftnoteasy Wangda Tan added a comment - Synced with Jonathan Hung offline, in additional to above suggestions, other changes: 1) Avoid call CapacityScheduler methods from provider. 2) Discard pending mutation during recovery. (Make sure that AdminService#refreshAll won't include the pending mutation too). 3) MutableScheduler#updateConfiguration can be removed. Instead we need an API to get confProvider.
          Hide
          jhung Jonathan Hung added a comment - - edited

          Thanks Wangda Tan, uploaded 006 patch:

          Then I suggest to add this as a Javadoc to the base class's method, this should be respected by all future implementations.

          Added

          Avoid call CapacityScheduler methods from provider.

          Done. Also exposed the ConfigurationMutationACLPolicy in MutableConfigurationProvider, since the policy calls cs.getQueue to check queue ACLs. (ACL checking is also done in RMWebServices just like the logging/refreshing/confirming.)

          Discard pending mutation during recovery. (Make sure that AdminService#refreshAll won't include the pending mutation too).

          Done. Also got rid of the transaction id logic, since there is only one pending mutation at any time. Also added some stuff in TestZKConfigurationStore#testFailoverReadsFromUpdatedStore to ensure pending mutation is not read on failover.

          MutableScheduler#updateConfiguration can be removed. Instead we need an API to get confProvider.

          Done, added aclPolicy/logging/confirming API to MutableConfigurationProvider

          Also since the pendingMutation/YarnConfigurationStore APIs changed, it's possible some of the LeveldbConfigurationStore logic broke, but I will address this in YARN-7046.

          Show
          jhung Jonathan Hung added a comment - - edited Thanks Wangda Tan , uploaded 006 patch: Then I suggest to add this as a Javadoc to the base class's method, this should be respected by all future implementations. Added Avoid call CapacityScheduler methods from provider. Done. Also exposed the ConfigurationMutationACLPolicy in MutableConfigurationProvider, since the policy calls cs.getQueue to check queue ACLs. (ACL checking is also done in RMWebServices just like the logging/refreshing/confirming.) Discard pending mutation during recovery. (Make sure that AdminService#refreshAll won't include the pending mutation too). Done. Also got rid of the transaction id logic, since there is only one pending mutation at any time. Also added some stuff in TestZKConfigurationStore#testFailoverReadsFromUpdatedStore to ensure pending mutation is not read on failover. MutableScheduler#updateConfiguration can be removed. Instead we need an API to get confProvider. Done, added aclPolicy/logging/confirming API to MutableConfigurationProvider Also since the pendingMutation/YarnConfigurationStore APIs changed, it's possible some of the LeveldbConfigurationStore logic broke, but I will address this in YARN-7046 .
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 15m 54s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
                YARN-5734 Compile Tests
          0 mvndep 2m 46s Maven dependency ordering for branch
          +1 mvninstall 16m 44s YARN-5734 passed
          +1 compile 9m 27s YARN-5734 passed
          +1 checkstyle 1m 1s YARN-5734 passed
          +1 mvnsite 1m 57s YARN-5734 passed
          +1 findbugs 3m 38s YARN-5734 passed
          +1 javadoc 1m 37s YARN-5734 passed
                Patch Compile Tests
          0 mvndep 0m 10s Maven dependency ordering for patch
          +1 mvninstall 1m 36s the patch passed
          +1 compile 5m 46s the patch passed
          +1 javac 5m 46s the patch passed
          -0 checkstyle 1m 0s hadoop-yarn-project/hadoop-yarn: The patch generated 8 new + 364 unchanged - 3 fixed = 372 total (was 367)
          +1 mvnsite 1m 55s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 3m 48s the patch passed
          -1 javadoc 0m 27s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 1 new + 347 unchanged - 0 fixed = 348 total (was 347)
                Other Tests
          +1 unit 0m 33s hadoop-yarn-api in the patch passed.
          +1 unit 2m 28s hadoop-yarn-common in the patch passed.
          -1 unit 44m 54s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 28s The patch does not generate ASF License warnings.
          126m 38s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation
            hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesConfigurationMutation
            hadoop.yarn.server.resourcemanager.scheduler.fair.TestFSAppStarvation



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6840
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12887235/YARN-6840-YARN-5734.006.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux 554b15275261 3.13.0-129-generic #178-Ubuntu SMP Fri Aug 11 12:48:20 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-5734 / 98c4a52
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17470/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          javadoc https://builds.apache.org/job/PreCommit-YARN-Build/17470/artifact/patchprocess/diff-javadoc-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/17470/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17470/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/17470/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 15m 54s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.       YARN-5734 Compile Tests 0 mvndep 2m 46s Maven dependency ordering for branch +1 mvninstall 16m 44s YARN-5734 passed +1 compile 9m 27s YARN-5734 passed +1 checkstyle 1m 1s YARN-5734 passed +1 mvnsite 1m 57s YARN-5734 passed +1 findbugs 3m 38s YARN-5734 passed +1 javadoc 1m 37s YARN-5734 passed       Patch Compile Tests 0 mvndep 0m 10s Maven dependency ordering for patch +1 mvninstall 1m 36s the patch passed +1 compile 5m 46s the patch passed +1 javac 5m 46s the patch passed -0 checkstyle 1m 0s hadoop-yarn-project/hadoop-yarn: The patch generated 8 new + 364 unchanged - 3 fixed = 372 total (was 367) +1 mvnsite 1m 55s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 3m 48s the patch passed -1 javadoc 0m 27s hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 1 new + 347 unchanged - 0 fixed = 348 total (was 347)       Other Tests +1 unit 0m 33s hadoop-yarn-api in the patch passed. +1 unit 2m 28s hadoop-yarn-common in the patch passed. -1 unit 44m 54s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 28s The patch does not generate ASF License warnings. 126m 38s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation   hadoop.yarn.server.resourcemanager.webapp.TestRMWebServicesConfigurationMutation   hadoop.yarn.server.resourcemanager.scheduler.fair.TestFSAppStarvation Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6840 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12887235/YARN-6840-YARN-5734.006.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux 554b15275261 3.13.0-129-generic #178-Ubuntu SMP Fri Aug 11 12:48:20 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-5734 / 98c4a52 Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17470/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt javadoc https://builds.apache.org/job/PreCommit-YARN-Build/17470/artifact/patchprocess/diff-javadoc-javadoc-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/17470/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17470/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/17470/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          Jonathan Hung,

          Thanks for updating the patch, latest patch LGTM, I can commit it once jenkins gets clean report.

          Could you please check TestRMWebServicesConfigurationMutation failure and javadocs warning?

          Show
          leftnoteasy Wangda Tan added a comment - Jonathan Hung , Thanks for updating the patch, latest patch LGTM, I can commit it once jenkins gets clean report. Could you please check TestRMWebServicesConfigurationMutation failure and javadocs warning?
          Hide
          jhung Jonathan Hung added a comment -

          Thanks Wangda Tan, attached 007 patch to fix javadoc/unit test.

          Show
          jhung Jonathan Hung added a comment - Thanks Wangda Tan , attached 007 patch to fix javadoc/unit test.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.
                YARN-5734 Compile Tests
          0 mvndep 2m 45s Maven dependency ordering for branch
          +1 mvninstall 16m 28s YARN-5734 passed
          +1 compile 9m 46s YARN-5734 passed
          +1 checkstyle 1m 2s YARN-5734 passed
          +1 mvnsite 2m 2s YARN-5734 passed
          +1 findbugs 3m 45s YARN-5734 passed
          +1 javadoc 1m 54s YARN-5734 passed
                Patch Compile Tests
          0 mvndep 0m 13s Maven dependency ordering for patch
          +1 mvninstall 1m 57s the patch passed
          +1 compile 7m 36s the patch passed
          +1 javac 7m 36s the patch passed
          -0 checkstyle 1m 26s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 364 unchanged - 3 fixed = 369 total (was 367)
          +1 mvnsite 2m 29s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 xml 0m 1s The patch has no ill-formed XML file.
          +1 findbugs 5m 25s the patch passed
          +1 javadoc 2m 9s the patch passed
                Other Tests
          +1 unit 0m 44s hadoop-yarn-api in the patch passed.
          +1 unit 2m 56s hadoop-yarn-common in the patch passed.
          -1 unit 48m 57s hadoop-yarn-server-resourcemanager in the patch failed.
          +1 asflicense 0m 30s The patch does not generate ASF License warnings.
          122m 8s



          Reason Tests
          Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation
            hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue YARN-6840
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12887408/YARN-6840-YARN-5734.007.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml
          uname Linux ddbd7d16e119 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision YARN-5734 / 98c4a52
          Default Java 1.8.0_144
          findbugs v3.1.0-RC1
          checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17482/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/17482/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17482/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/17482/console
          Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 5 new or modified test files.       YARN-5734 Compile Tests 0 mvndep 2m 45s Maven dependency ordering for branch +1 mvninstall 16m 28s YARN-5734 passed +1 compile 9m 46s YARN-5734 passed +1 checkstyle 1m 2s YARN-5734 passed +1 mvnsite 2m 2s YARN-5734 passed +1 findbugs 3m 45s YARN-5734 passed +1 javadoc 1m 54s YARN-5734 passed       Patch Compile Tests 0 mvndep 0m 13s Maven dependency ordering for patch +1 mvninstall 1m 57s the patch passed +1 compile 7m 36s the patch passed +1 javac 7m 36s the patch passed -0 checkstyle 1m 26s hadoop-yarn-project/hadoop-yarn: The patch generated 5 new + 364 unchanged - 3 fixed = 369 total (was 367) +1 mvnsite 2m 29s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 findbugs 5m 25s the patch passed +1 javadoc 2m 9s the patch passed       Other Tests +1 unit 0m 44s hadoop-yarn-api in the patch passed. +1 unit 2m 56s hadoop-yarn-common in the patch passed. -1 unit 48m 57s hadoop-yarn-server-resourcemanager in the patch failed. +1 asflicense 0m 30s The patch does not generate ASF License warnings. 122m 8s Reason Tests Failed junit tests hadoop.yarn.server.resourcemanager.scheduler.capacity.TestContainerAllocation   hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue YARN-6840 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12887408/YARN-6840-YARN-5734.007.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml uname Linux ddbd7d16e119 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision YARN-5734 / 98c4a52 Default Java 1.8.0_144 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/17482/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/17482/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/17482/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn Console output https://builds.apache.org/job/PreCommit-YARN-Build/17482/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          leftnoteasy Wangda Tan added a comment -

          +1, will commit today. Thanks Jonathan Hung

          Show
          leftnoteasy Wangda Tan added a comment - +1, will commit today. Thanks Jonathan Hung
          Hide
          leftnoteasy Wangda Tan added a comment -

          Committed to branch, thanks Jonathan Hung and thanks reviews from Xuan Gong / Subru Krishnan / Daniel Templeton / Inigo / Kai Zheng / Naganarasimha G R / Zhe Zhang!

          Show
          leftnoteasy Wangda Tan added a comment - Committed to branch, thanks Jonathan Hung and thanks reviews from Xuan Gong / Subru Krishnan / Daniel Templeton / Inigo / Kai Zheng / Naganarasimha G R / Zhe Zhang !
          Hide
          jhung Jonathan Hung added a comment -

          Thanks all!

          Show
          jhung Jonathan Hung added a comment - Thanks all!

            People

            • Assignee:
              jhung Jonathan Hung
              Reporter:
              leftnoteasy Wangda Tan
            • Votes:
              0 Vote for this issue
              Watchers:
              16 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development