Uploaded image for project: 'Hadoop YARN'
  1. Hadoop YARN
  2. YARN-5704

Provide config knobs to control enabling/disabling new/work in progress features in container-executor

    Details

    • Type: Task
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.8.0, 3.0.0-alpha1
    • Fix Version/s: 2.8.0, 3.0.0-alpha2
    • Component/s: yarn
    • Labels:
      None

      Description

      Provide a mechanism to enable/disable Docker and TC (Traffic Control) functionality at the container-executor level.

      1. YARN-5704.001.patch
        18 kB
        Sidharta Seethana
      2. YARN-5704-branch-2.8.001.patch
        17 kB
        Sidharta Seethana

        Issue Links

          Activity

          Hide
          sidharta-s Sidharta Seethana added a comment -

          Uploaded a patch that provides config knobs to enable/disable docker/tc functionality. The usage message is also updated accordingly. I tested various enable/disable combinations manually on Centos 7.2.

          Show
          sidharta-s Sidharta Seethana added a comment - Uploaded a patch that provides config knobs to enable/disable docker/tc functionality. The usage message is also updated accordingly. I tested various enable/disable combinations manually on Centos 7.2.
          Hide
          sidharta-s Sidharta Seethana added a comment -

          Submitted to Jenkins. Varun Vasudev, could you please take a look?

          Show
          sidharta-s Sidharta Seethana added a comment - Submitted to Jenkins. Varun Vasudev , could you please take a look?
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 7m 35s trunk passed
          +1 compile 0m 36s trunk passed
          +1 mvnsite 0m 30s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 mvninstall 0m 24s the patch passed
          +1 compile 0m 25s the patch passed
          +1 cc 0m 25s the patch passed
          +1 javac 0m 25s the patch passed
          +1 mvnsite 0m 28s the patch passed
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 unit 15m 9s hadoop-yarn-server-nodemanager in the patch passed.
          +1 asflicense 0m 15s The patch does not generate ASF License warnings.
          26m 20s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:9560f25
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831468/YARN-5704.001.patch
          JIRA Issue YARN-5704
          Optional Tests asflicense compile cc mvnsite javac unit
          uname Linux fe50de5dffa5 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / f61e3d1
          Default Java 1.8.0_101
          Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13273/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13273/console
          Powered by Apache Yetus 0.3.0 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. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 7m 35s trunk passed +1 compile 0m 36s trunk passed +1 mvnsite 0m 30s trunk passed +1 mvneclipse 0m 12s trunk passed +1 mvninstall 0m 24s the patch passed +1 compile 0m 25s the patch passed +1 cc 0m 25s the patch passed +1 javac 0m 25s the patch passed +1 mvnsite 0m 28s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 unit 15m 9s hadoop-yarn-server-nodemanager in the patch passed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 26m 20s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12831468/YARN-5704.001.patch JIRA Issue YARN-5704 Optional Tests asflicense compile cc mvnsite javac unit uname Linux fe50de5dffa5 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / f61e3d1 Default Java 1.8.0_101 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13273/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/13273/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          vvasudev Varun Vasudev added a comment -

          +1. Committing to trunk and branch-2.

          Show
          vvasudev Varun Vasudev added a comment - +1. Committing to trunk and branch-2.
          Hide
          vvasudev Varun Vasudev added a comment -

          Sidharta Seethana - the patch doesn't apply cleanly to branch-2.8 or branch-2.7.3. Can you please provide versions for them? I've committed it to trunk and branch-2.

          Show
          vvasudev Varun Vasudev added a comment - Sidharta Seethana - the patch doesn't apply cleanly to branch-2.8 or branch-2.7.3. Can you please provide versions for them? I've committed it to trunk and branch-2.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10543 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10543/)
          YARN-5704. Provide config knobs to control enabling/disabling new/work (vvasudev: rev 0992708d790b5bd3dab85987b7ad7c6fc2cc4b18)

          • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/main.c
          • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.h
          • (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.c
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10543 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10543/ ) YARN-5704 . Provide config knobs to control enabling/disabling new/work (vvasudev: rev 0992708d790b5bd3dab85987b7ad7c6fc2cc4b18) (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/main.c (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.h (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/container-executor.c
          Hide
          aw Allen Wittenauer added a comment -

          I tested various enable/disable combinations manually on Centos 7.2.

          Why weren't tests added to test-container-executor?

          Show
          aw Allen Wittenauer added a comment - I tested various enable/disable combinations manually on Centos 7.2. Why weren't tests added to test-container-executor?
          Hide
          sidharta-s Sidharta Seethana added a comment -

          Allen Wittenauer As you can see from the patch, most of the changes in the patch are in main.c . Changes to this file cannot be tested via test-container-executor, I believe ?

          Show
          sidharta-s Sidharta Seethana added a comment - Allen Wittenauer As you can see from the patch, most of the changes in the patch are in main.c . Changes to this file cannot be tested via test-container-executor, I believe ?
          Hide
          sidharta-s Sidharta Seethana added a comment -

          Varun Vasudev, this patch needs YARN-4749 to be backported into branch-2.8 and branch-2.7 (that patch was committed as YARN-4245) .

          Show
          sidharta-s Sidharta Seethana added a comment - Varun Vasudev , this patch needs YARN-4749 to be backported into branch-2.8 and branch-2.7 (that patch was committed as YARN-4245 ) .
          Hide
          sidharta-s Sidharta Seethana added a comment -

          Uploaded a patch for branch-2.8.

          Show
          sidharta-s Sidharta Seethana added a comment - Uploaded a patch for branch-2.8.
          Hide
          sidharta-s Sidharta Seethana added a comment -

          Varun Vasudev, could you please take a look? Also, It looks like neither of these features are in branch-2.7 so a patch isn't necessary for that branch.

          Show
          sidharta-s Sidharta Seethana added a comment - Varun Vasudev , could you please take a look? Also, It looks like neither of these features are in branch-2.7 so a patch isn't necessary for that branch.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 20s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 6m 53s branch-2.8 passed
          +1 compile 0m 35s branch-2.8 passed with JDK v1.8.0_101
          +1 compile 0m 34s branch-2.8 passed with JDK v1.7.0_111
          +1 mvnsite 0m 36s branch-2.8 passed
          +1 mvneclipse 0m 16s branch-2.8 passed
          +1 mvninstall 0m 26s the patch passed
          -1 compile 0m 18s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.8.0_101.
          -1 cc 0m 18s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.8.0_101.
          -1 javac 0m 18s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.8.0_101.
          -1 compile 0m 21s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.7.0_111.
          -1 cc 0m 21s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.7.0_111.
          -1 javac 0m 21s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.7.0_111.
          +1 mvnsite 0m 30s the patch passed
          +1 mvneclipse 0m 12s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          -1 unit 0m 18s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.8.0_101.
          -1 unit 0m 23s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.7.0_111.
          +1 asflicense 0m 21s The patch does not generate ASF License warnings.
          12m 30s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:5af2af1
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12832099/YARN-5704-branch-2.8.001.patch
          JIRA Issue YARN-5704
          Optional Tests asflicense compile cc mvnsite javac unit
          uname Linux 677e0815ce53 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision branch-2.8 / 7296999
          Default Java 1.7.0_111
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_101 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_111
          compile https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.8.0_101.txt
          cc https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.8.0_101.txt
          javac https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.8.0_101.txt
          compile https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.7.0_111.txt
          cc https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.7.0_111.txt
          javac https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.7.0_111.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.8.0_101.txt
          unit https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.7.0_111.txt
          JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13315/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13315/console
          Powered by Apache Yetus 0.3.0 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 20s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 6m 53s branch-2.8 passed +1 compile 0m 35s branch-2.8 passed with JDK v1.8.0_101 +1 compile 0m 34s branch-2.8 passed with JDK v1.7.0_111 +1 mvnsite 0m 36s branch-2.8 passed +1 mvneclipse 0m 16s branch-2.8 passed +1 mvninstall 0m 26s the patch passed -1 compile 0m 18s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.8.0_101. -1 cc 0m 18s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.8.0_101. -1 javac 0m 18s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.8.0_101. -1 compile 0m 21s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.7.0_111. -1 cc 0m 21s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.7.0_111. -1 javac 0m 21s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.7.0_111. +1 mvnsite 0m 30s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. -1 unit 0m 18s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.8.0_101. -1 unit 0m 23s hadoop-yarn-server-nodemanager in the patch failed with JDK v1.7.0_111. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 12m 30s Subsystem Report/Notes Docker Image:yetus/hadoop:5af2af1 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12832099/YARN-5704-branch-2.8.001.patch JIRA Issue YARN-5704 Optional Tests asflicense compile cc mvnsite javac unit uname Linux 677e0815ce53 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision branch-2.8 / 7296999 Default Java 1.7.0_111 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_101 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_111 compile https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.8.0_101.txt cc https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.8.0_101.txt javac https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.8.0_101.txt compile https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.7.0_111.txt cc https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.7.0_111.txt javac https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-compile-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.7.0_111.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.8.0_101.txt unit https://builds.apache.org/job/PreCommit-YARN-Build/13315/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager-jdk1.7.0_111.txt JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13315/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/13315/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          aw Allen Wittenauer added a comment -

          As you can see from the patch, most of the changes in the patch are in main.c .

          Most, but not all. The single most important thing this code adds is the is_feature_enabled function and it should definitely have a test.

          Show
          aw Allen Wittenauer added a comment - As you can see from the patch, most of the changes in the patch are in main.c . Most, but not all. The single most important thing this code adds is the is_feature_enabled function and it should definitely have a test.
          Hide
          vvasudev Varun Vasudev added a comment -

          I backported YARN-4749 to branch-2.8 and have kicked off Jenkins manually.

          Show
          vvasudev Varun Vasudev added a comment - I backported YARN-4749 to branch-2.8 and have kicked off Jenkins manually.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 19s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 6m 40s branch-2.8 passed
          +1 compile 0m 27s branch-2.8 passed with JDK v1.8.0_101
          +1 compile 0m 29s branch-2.8 passed with JDK v1.7.0_111
          +1 mvnsite 0m 28s branch-2.8 passed
          +1 mvneclipse 0m 14s branch-2.8 passed
          +1 mvninstall 0m 23s the patch passed
          +1 compile 0m 21s the patch passed with JDK v1.8.0_101
          +1 cc 0m 21s the patch passed
          +1 javac 0m 21s the patch passed
          +1 compile 0m 26s the patch passed with JDK v1.7.0_111
          +1 cc 0m 26s the patch passed
          +1 javac 0m 26s the patch passed
          +1 mvnsite 0m 26s the patch passed
          +1 mvneclipse 0m 10s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 unit 9m 6s hadoop-yarn-server-nodemanager in the patch passed with JDK v1.8.0_101.
          +1 unit 9m 38s hadoop-yarn-server-nodemanager in the patch passed with JDK v1.7.0_111.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          29m 50s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:5af2af1
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12832099/YARN-5704-branch-2.8.001.patch
          JIRA Issue YARN-5704
          Optional Tests asflicense compile cc mvnsite javac unit
          uname Linux 7c7ed146754d 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision branch-2.8 / 5040940
          Default Java 1.7.0_111
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_101 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_111
          JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13327/testReport/
          modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager
          Console output https://builds.apache.org/job/PreCommit-YARN-Build/13327/console
          Powered by Apache Yetus 0.3.0 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 19s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 6m 40s branch-2.8 passed +1 compile 0m 27s branch-2.8 passed with JDK v1.8.0_101 +1 compile 0m 29s branch-2.8 passed with JDK v1.7.0_111 +1 mvnsite 0m 28s branch-2.8 passed +1 mvneclipse 0m 14s branch-2.8 passed +1 mvninstall 0m 23s the patch passed +1 compile 0m 21s the patch passed with JDK v1.8.0_101 +1 cc 0m 21s the patch passed +1 javac 0m 21s the patch passed +1 compile 0m 26s the patch passed with JDK v1.7.0_111 +1 cc 0m 26s the patch passed +1 javac 0m 26s the patch passed +1 mvnsite 0m 26s the patch passed +1 mvneclipse 0m 10s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 unit 9m 6s hadoop-yarn-server-nodemanager in the patch passed with JDK v1.8.0_101. +1 unit 9m 38s hadoop-yarn-server-nodemanager in the patch passed with JDK v1.7.0_111. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 29m 50s Subsystem Report/Notes Docker Image:yetus/hadoop:5af2af1 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12832099/YARN-5704-branch-2.8.001.patch JIRA Issue YARN-5704 Optional Tests asflicense compile cc mvnsite javac unit uname Linux 7c7ed146754d 3.13.0-95-generic #142-Ubuntu SMP Fri Aug 12 17:00:09 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision branch-2.8 / 5040940 Default Java 1.7.0_111 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_101 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_111 JDK v1.7.0_111 Test Results https://builds.apache.org/job/PreCommit-YARN-Build/13327/testReport/ modules C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager Console output https://builds.apache.org/job/PreCommit-YARN-Build/13327/console Powered by Apache Yetus 0.3.0 http://yetus.apache.org This message was automatically generated.
          Hide
          sidharta-s Sidharta Seethana added a comment -

          Allen Wittenauer, the function you are referring to is static currently - and is not visible outside of that file and hence cannot be tested via test-container-executor either. I have filed YARN-5717 to make this function public and add tests for it. I'll follow up soon with a patch - I am hoping you could help review/commit it.

          Varun Vasudev, if the branch-2.8 version of patch looks ok to you, could you please go ahead and commit it?

          Show
          sidharta-s Sidharta Seethana added a comment - Allen Wittenauer , the function you are referring to is static currently - and is not visible outside of that file and hence cannot be tested via test-container-executor either. I have filed YARN-5717 to make this function public and add tests for it. I'll follow up soon with a patch - I am hoping you could help review/commit it. Varun Vasudev , if the branch-2.8 version of patch looks ok to you, could you please go ahead and commit it?
          Hide
          vvasudev Varun Vasudev added a comment -

          Sidharta Seethana - a new JIRA makes sense just to make review easier(given the commit to trunk and branch-2). I've marked YARN-5717 as a blocker for this JIRA and committed the patch you provided to branch-2.8 as well.

          Show
          vvasudev Varun Vasudev added a comment - Sidharta Seethana - a new JIRA makes sense just to make review easier(given the commit to trunk and branch-2). I've marked YARN-5717 as a blocker for this JIRA and committed the patch you provided to branch-2.8 as well.
          Hide
          aw Allen Wittenauer added a comment -

          function you are referring to is static currently

          Frankly, that's the least of this patch's problems. It features almost all of the issues that currently plague c-e:

          1. Lack of a unit test. Covered.

          2. Useless abstraction:

          int is_docker_support_enabled() {
              return is_feature_enabled(DOCKER_SUPPORT_ENABLED_KEY,
              DEFAULT_DOCKER_SUPPORT_ENABLED);
          }
          
          int is_tc_support_enabled() {
              return is_feature_enabled(TC_SUPPORT_ENABLED_KEY,
              DEFAULT_TC_SUPPORT_ENABLED);
          }
          

          3. Variable declaration in the middle:

                  char *end_ptr = NULL;
          
          Show
          aw Allen Wittenauer added a comment - function you are referring to is static currently Frankly, that's the least of this patch's problems. It features almost all of the issues that currently plague c-e: 1. Lack of a unit test. Covered. 2. Useless abstraction: int is_docker_support_enabled() { return is_feature_enabled(DOCKER_SUPPORT_ENABLED_KEY, DEFAULT_DOCKER_SUPPORT_ENABLED); } int is_tc_support_enabled() { return is_feature_enabled(TC_SUPPORT_ENABLED_KEY, DEFAULT_TC_SUPPORT_ENABLED); } 3. Variable declaration in the middle: char *end_ptr = NULL;
          Hide
          sidharta-s Sidharta Seethana added a comment -

          /cc Chris Douglas, Vinod Kumar Vavilapalli

          Allen Wittenauer, going back and forth discussing new concerns every time is not productive. Do you have more concerns with this patch or is the list above complete? Regarding your comments above :

          1. I explained already that most of the changes are in main.c which cannot be tested via test-container-executor - I'll go ahead add tests for the function you referred to.
          2. I don't believe it is a "useless abstraction" - those functions provide useful, less error-prone shorthands.
          3. “In the middle” ? This variable is declared at the beginning of a new scope. Even if it weren’t, declaring variables ‘near’ where they are needed is common practice and is a widely used GNU extension. If this is a problem, we could use static checking as part of our build or add the -ansi and -pedantic compiler flags (and in the process break code that has existed for 5+ years).
          Show
          sidharta-s Sidharta Seethana added a comment - /cc Chris Douglas , Vinod Kumar Vavilapalli Allen Wittenauer , going back and forth discussing new concerns every time is not productive. Do you have more concerns with this patch or is the list above complete? Regarding your comments above : I explained already that most of the changes are in main.c which cannot be tested via test-container-executor - I'll go ahead add tests for the function you referred to. I don't believe it is a "useless abstraction" - those functions provide useful, less error-prone shorthands. “In the middle” ? This variable is declared at the beginning of a new scope. Even if it weren’t, declaring variables ‘near’ where they are needed is common practice and is a widely used GNU extension. If this is a problem, we could use static checking as part of our build or add the -ansi and -pedantic compiler flags (and in the process break code that has existed for 5+ years).
          Hide
          aw Allen Wittenauer added a comment -

          Look, if I was super concerned about those other problems in the patch, I would have brought them up in the first post. I just found it really off putting that your major reason for not building a unit test was because it was declared static. It's like... really? If we take that to it's logical conclusion we just declare all of our utility functions as static and remove all the unit tests.

          and in the process break code that has existed for 5+ years

          ... which is why I said...

          It features almost all of the issues that currently plague c-e

          Just because the old code follows bad practices doesn't mean that new code should. c-e not being ANSI C compliant is a problem, BTW.

          Show
          aw Allen Wittenauer added a comment - Look, if I was super concerned about those other problems in the patch, I would have brought them up in the first post. I just found it really off putting that your major reason for not building a unit test was because it was declared static . It's like... really? If we take that to it's logical conclusion we just declare all of our utility functions as static and remove all the unit tests. and in the process break code that has existed for 5+ years ... which is why I said... It features almost all of the issues that currently plague c-e Just because the old code follows bad practices doesn't mean that new code should. c-e not being ANSI C compliant is a problem, BTW.
          Hide
          sidharta-s Sidharta Seethana added a comment -

          Allen Wittenauer, again : my reason for testing this manually was that most of the changes were to main.c - that was the only way to test if enabling/disabling these features worked in the first place. The fact that the function was static ( and the fact that the patch was already committed to trunk/branch-2) were the reasons for filing a separate JIRA for the tests to keep things clean - I was not using that as a reason to not add tests. container-executor in general needs a complete overhaul - for testability if not anything else.

          There is no disagreement that bad practices should not be further propagated - but we disagree on 'where variables should be declared in C code' being one among them.

          Show
          sidharta-s Sidharta Seethana added a comment - Allen Wittenauer , again : my reason for testing this manually was that most of the changes were to main.c - that was the only way to test if enabling/disabling these features worked in the first place. The fact that the function was static ( and the fact that the patch was already committed to trunk/branch-2) were the reasons for filing a separate JIRA for the tests to keep things clean - I was not using that as a reason to not add tests. container-executor in general needs a complete overhaul - for testability if not anything else. There is no disagreement that bad practices should not be further propagated - but we disagree on 'where variables should be declared in C code' being one among them.
          Hide
          chris.douglas Chris Douglas added a comment -

          Thanks for working on this, Sidharta Seethana.

          As part of the followup patch, please also avoid using strcat when printing the usage can be separated into multiple statements. Avoids allocating a buffer we need to track for overflow. Sorry, I hadn't noticed that earlier.

          If we take that to it's logical conclusion we just declare all of our utility functions as static and remove all the unit tests.

          That takes this heuristic well past its logical conclusion, but it'll be addressed in YARN-5717.

          [Variable declaration in the middle] Just because the old code follows bad practices doesn't mean that new code should. c-e not being ANSI C compliant is a problem, BTW.

          If this creates portability problems that makes sense, though VS is the only C compiler I know of that (until recently?) doesn't support most of C99. Initializing variables when they're declared can avoid accidents, particularly over long LCE methods. Are any platforms this could target restricted to ANSI C compilers?

          Requiring that new patches use ANSI C, without making the rest of LCE compliant, adds a touchy manual step for committers and helps no users. If there are restrictions on the subset of C this should use, the compiler needs to enforce them.

          Show
          chris.douglas Chris Douglas added a comment - Thanks for working on this, Sidharta Seethana . As part of the followup patch, please also avoid using strcat when printing the usage can be separated into multiple statements. Avoids allocating a buffer we need to track for overflow. Sorry, I hadn't noticed that earlier. If we take that to it's logical conclusion we just declare all of our utility functions as static and remove all the unit tests. That takes this heuristic well past its logical conclusion, but it'll be addressed in YARN-5717 . [Variable declaration in the middle] Just because the old code follows bad practices doesn't mean that new code should. c-e not being ANSI C compliant is a problem, BTW. If this creates portability problems that makes sense, though VS is the only C compiler I know of that (until recently?) doesn't support most of C99. Initializing variables when they're declared can avoid accidents, particularly over long LCE methods. Are any platforms this could target restricted to ANSI C compilers? Requiring that new patches use ANSI C, without making the rest of LCE compliant, adds a touchy manual step for committers and helps no users. If there are restrictions on the subset of C this should use, the compiler needs to enforce them.
          Hide
          aw Allen Wittenauer added a comment -

          though VS is the only C compiler I know of that (until recently?) doesn't support most of C99

          It's a bit more complex than that. Let's dig into the details.

          GNU, as usual, does whatever the hell GNU wants. Unless one tells it otherwise, gcc's supported features changes pretty much every release and tends to be wildly unpredictable. This can result in some interesting side effects for advanced programmers. e.g., tricks to build custom memory allocators were recently 'optimized' away.

          Most non-GNU-based compilers that I've worked with default (usually) to C89 with extensions (e.g., C++ style comments). This is primarily due to C99 being incompatible with previous C standards. (new keywords, int declarations, ...) In order to tell the compiler that no, really, we want to use C99 and not to barf on the incompatbilities, then (minimally) a flag has to get passed or (maximally) a different compiler executable (hi DPW) needs to get used. If we want to declare this code base as being C99 then we need to tell cmake to make sure we're using a C99 compiler. Until we do that, this code is defaulting to non-C99.

          In addition, we would also need to audit the code to make sure that we really are writing C99. That shouldn't be too difficult, but it is something that needs to happen. Minimally, compiling with something that isn't gcc in C99 mode would likely highlight the problem areas since gcc tends to be... forgiving.... without --pedantic.

          Bonus: telling cmake that we're doing C99 is sort of a mine field, depending upon which version of cmake is in use.

          If there are restrictions on the subset of C this should use, the compiler needs to enforce them.

          Totally agree. But the end result is we can't just declare we're writing C99 and then expect the compiler to magically know that.

          Anyway...

          As I said above, my #1 issue with this patch is the lack of a unit test. If someone wrote a method that determined if experimental/insecure features were enabled in the heart of the Java authentication code based upon a site.xml setting, I'm fairly confident that the contributor would be adding and anyone reviewing would flag the need for even a basic unit test, if not true, false, & broken input tests. In one sense, I understand why the community at large treats the non-Java code as coming from a lower caste, but c-e is one of the most important parts of the source base. It really does require extra scrutiny and the lack of even a basic unit test when it's clearly possible should be a huge red flag.

          Show
          aw Allen Wittenauer added a comment - though VS is the only C compiler I know of that (until recently?) doesn't support most of C99 It's a bit more complex than that. Let's dig into the details. GNU, as usual, does whatever the hell GNU wants. Unless one tells it otherwise, gcc's supported features changes pretty much every release and tends to be wildly unpredictable. This can result in some interesting side effects for advanced programmers. e.g., tricks to build custom memory allocators were recently 'optimized' away. Most non-GNU-based compilers that I've worked with default (usually) to C89 with extensions (e.g., C++ style comments). This is primarily due to C99 being incompatible with previous C standards. (new keywords, int declarations, ...) In order to tell the compiler that no, really, we want to use C99 and not to barf on the incompatbilities, then (minimally) a flag has to get passed or (maximally) a different compiler executable (hi DPW) needs to get used. If we want to declare this code base as being C99 then we need to tell cmake to make sure we're using a C99 compiler. Until we do that, this code is defaulting to non-C99. In addition, we would also need to audit the code to make sure that we really are writing C99. That shouldn't be too difficult, but it is something that needs to happen. Minimally, compiling with something that isn't gcc in C99 mode would likely highlight the problem areas since gcc tends to be... forgiving.... without --pedantic. Bonus: telling cmake that we're doing C99 is sort of a mine field, depending upon which version of cmake is in use. If there are restrictions on the subset of C this should use, the compiler needs to enforce them. Totally agree. But the end result is we can't just declare we're writing C99 and then expect the compiler to magically know that. Anyway... As I said above, my #1 issue with this patch is the lack of a unit test. If someone wrote a method that determined if experimental/insecure features were enabled in the heart of the Java authentication code based upon a site.xml setting, I'm fairly confident that the contributor would be adding and anyone reviewing would flag the need for even a basic unit test, if not true, false, & broken input tests. In one sense, I understand why the community at large treats the non-Java code as coming from a lower caste, but c-e is one of the most important parts of the source base. It really does require extra scrutiny and the lack of even a basic unit test when it's clearly possible should be a huge red flag.
          Hide
          chris.douglas Chris Douglas added a comment -

          If we want to declare this code base as being C99 then we need to tell cmake to make sure we're using a C99 compiler. Until we do that, this code is defaulting to non-C99.

          OK, got it. I don't suppose NoC99 has the same cachet as NoSQL? Let's pick a standard. Filed YARN-5719

          telling cmake that we're doing C99 is sort of a mine field, depending upon which version of cmake is in use.

          Took a look at this and... yikes.

          Show
          chris.douglas Chris Douglas added a comment - If we want to declare this code base as being C99 then we need to tell cmake to make sure we're using a C99 compiler. Until we do that, this code is defaulting to non-C99. OK, got it. I don't suppose NoC99 has the same cachet as NoSQL? Let's pick a standard. Filed YARN-5719 telling cmake that we're doing C99 is sort of a mine field, depending upon which version of cmake is in use. Took a look at this and... yikes.
          Hide
          vvasudev Varun Vasudev added a comment -

          YARN-5717 has been committed. Closing this JIRA. Thanks for the patch Sidharta Seethana!

          Show
          vvasudev Varun Vasudev added a comment - YARN-5717 has been committed. Closing this JIRA. Thanks for the patch Sidharta Seethana !
          Hide
          sidharta-s Sidharta Seethana added a comment -

          Thanks, Varun Vasudev.

          Show
          sidharta-s Sidharta Seethana added a comment - Thanks, Varun Vasudev .
          Hide
          chris.douglas Chris Douglas added a comment -

          Varun Vasudev would you mind taking a look at YARN-5719 so we can enforce C99 (or whatever) for CE?

          Show
          chris.douglas Chris Douglas added a comment - Varun Vasudev would you mind taking a look at YARN-5719 so we can enforce C99 (or whatever) for CE?

            People

            • Assignee:
              sidharta-s Sidharta Seethana
              Reporter:
              sidharta-s Sidharta Seethana
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development