Skip to content

Conversation

@ajayydv
Copy link
Contributor

@ajayydv ajayydv commented Mar 12, 2019

…tainerManagerHttpServer. Contributed by Ajay Kumar.

@ajayydv ajayydv added the ozone label Mar 12, 2019
@ajayydv ajayydv self-assigned this Mar 12, 2019
@ajayydv ajayydv requested a review from xiaoyuyao March 12, 2019 19:31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the last assert in line 113-114 be changed to for http?

  Assert.assertTrue(implies(policy.isHttpEnabled(),
           !canAccess("http", server.getHttpAddress())));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested assert seems incorrect coz if http is enabled than client should succeed in binding to http port.

We do assert if http is enabled, server should bind to http port and client should be able to connect to it. in L105

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. Then in line 113-114, we are trying to validate that if https is not enabled, you can't use http scheme to access an https address?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as below

Copy link
Contributor

@xiaoyuyao xiaoyuyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall. Just have one minor question inline.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 27 Docker mode activated.
_ Prechecks _
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 2 new or modified test files.
_ trunk Compile Tests _
0 mvndep 62 Maven dependency ordering for branch
+1 mvninstall 999 trunk passed
+1 compile 956 trunk passed
+1 checkstyle 190 trunk passed
-1 mvnsite 45 ozone-manager in trunk failed.
+1 shadedclient 1007 branch has no errors when building and testing our client artifacts.
-1 findbugs 36 ozone-manager in trunk failed.
+1 javadoc 67 trunk passed
_ Patch Compile Tests _
0 mvndep 22 Maven dependency ordering for patch
-1 mvninstall 19 ozone-manager in the patch failed.
+1 compile 902 the patch passed
+1 javac 902 the patch passed
+1 checkstyle 189 the patch passed
-1 mvnsite 35 ozone-manager in the patch failed.
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 669 patch has no errors when building and testing our client artifacts.
-1 findbugs 35 ozone-manager in the patch failed.
+1 javadoc 66 the patch passed
_ Other Tests _
+1 unit 113 server-scm in the patch passed.
-1 unit 37 ozone-manager in the patch failed.
+1 asflicense 47 The patch does not generate ASF License warnings.
5731
Subsystem Report/Notes
Docker Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-598/1/artifact/out/Dockerfile
GITHUB PR #598
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux 5f690d71399f 4.4.0-138-generic #164-Ubuntu SMP Tue Oct 2 17:16:02 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 24793d2
maven version: Apache Maven 3.3.9
Default Java 1.8.0_191
mvnsite https://builds.apache.org/job/hadoop-multibranch/job/PR-598/1/artifact/out/branch-mvnsite-hadoop-ozone_ozone-manager.txt
findbugs v3.1.0-RC1
findbugs https://builds.apache.org/job/hadoop-multibranch/job/PR-598/1/artifact/out/branch-findbugs-hadoop-ozone_ozone-manager.txt
mvninstall https://builds.apache.org/job/hadoop-multibranch/job/PR-598/1/artifact/out/patch-mvninstall-hadoop-ozone_ozone-manager.txt
mvnsite https://builds.apache.org/job/hadoop-multibranch/job/PR-598/1/artifact/out/patch-mvnsite-hadoop-ozone_ozone-manager.txt
findbugs https://builds.apache.org/job/hadoop-multibranch/job/PR-598/1/artifact/out/patch-findbugs-hadoop-ozone_ozone-manager.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-598/1/artifact/out/patch-unit-hadoop-ozone_ozone-manager.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-598/1/testReport/
Max. process+thread count 446 (vs. ulimit of 5500)
modules C: hadoop-hdds/server-scm hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-598/1/console
Powered by Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

…tainerManagerHttpServer. Contributed by Ajay Kumar.
@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 35 Docker mode activated.
_ Prechecks _
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 2 new or modified test files.
_ trunk Compile Tests _
0 mvndep 29 Maven dependency ordering for branch
+1 mvninstall 1132 trunk passed
+1 compile 1031 trunk passed
+1 checkstyle 199 trunk passed
-1 mvnsite 40 ozone-manager in trunk failed.
+1 shadedclient 1060 branch has no errors when building and testing our client artifacts.
-1 findbugs 32 ozone-manager in trunk failed.
+1 javadoc 60 trunk passed
_ Patch Compile Tests _
0 mvndep 20 Maven dependency ordering for patch
-1 mvninstall 22 ozone-manager in the patch failed.
+1 compile 1192 the patch passed
+1 javac 1192 the patch passed
+1 checkstyle 215 the patch passed
-1 mvnsite 34 ozone-manager in the patch failed.
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 793 patch has no errors when building and testing our client artifacts.
-1 findbugs 38 ozone-manager in the patch failed.
+1 javadoc 90 the patch passed
_ Other Tests _
+1 unit 138 server-scm in the patch passed.
-1 unit 38 ozone-manager in the patch failed.
+1 asflicense 56 The patch does not generate ASF License warnings.
6469
Subsystem Report/Notes
Docker Client=17.05.0-ce Server=17.05.0-ce base: https://builds.apache.org/job/hadoop-multibranch/job/PR-598/2/artifact/out/Dockerfile
GITHUB PR #598
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux 2a9b37a66499 3.13.0-153-generic #203-Ubuntu SMP Thu Jun 14 08:52:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / 8e1539e
maven version: Apache Maven 3.3.9
Default Java 1.8.0_191
mvnsite https://builds.apache.org/job/hadoop-multibranch/job/PR-598/2/artifact/out/branch-mvnsite-hadoop-ozone_ozone-manager.txt
findbugs v3.1.0-RC1
findbugs https://builds.apache.org/job/hadoop-multibranch/job/PR-598/2/artifact/out/branch-findbugs-hadoop-ozone_ozone-manager.txt
mvninstall https://builds.apache.org/job/hadoop-multibranch/job/PR-598/2/artifact/out/patch-mvninstall-hadoop-ozone_ozone-manager.txt
mvnsite https://builds.apache.org/job/hadoop-multibranch/job/PR-598/2/artifact/out/patch-mvnsite-hadoop-ozone_ozone-manager.txt
findbugs https://builds.apache.org/job/hadoop-multibranch/job/PR-598/2/artifact/out/patch-findbugs-hadoop-ozone_ozone-manager.txt
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-598/2/artifact/out/patch-unit-hadoop-ozone_ozone-manager.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-598/2/testReport/
Max. process+thread count 366 (vs. ulimit of 5500)
modules C: hadoop-hdds/server-scm hadoop-ozone/ozone-manager U: .
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-598/2/console
Powered by Apache Yetus 0.9.0 http://yetus.apache.org

This message was automatically generated.

@jnp
Copy link
Contributor

jnp commented Mar 13, 2019

+1 for the patch.

@ajayydv ajayydv merged commit 4fa0099 into apache:trunk Mar 13, 2019
ajayydv added a commit that referenced this pull request Mar 13, 2019
…tainerManagerHttpServer. Contributed by Ajay Kumar. (#598)

(cherry picked from commit 4fa0099)
@ajayydv ajayydv deleted the HDDS-1254 branch March 21, 2019 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants