Description
Attachments
Attachments
- YARN-8126.002.patch
- 22 kB
- Rohith Sharma K S
- YARN-8126.001.patch
- 20 kB
- Rohith Sharma K S
Issue Links
- relates to
-
YARN-8048 Support auto-spawning of admin configured services during bootstrap of rm/apiserver
- Resolved
Activity
+1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 29s | 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 14 new or modified test files. |
trunk Compile Tests | |||
0 | mvndep | 0m 10s | Maven dependency ordering for branch |
+1 | mvninstall | 25m 33s | trunk passed |
+1 | compile | 8m 53s | trunk passed |
+1 | checkstyle | 1m 19s | trunk passed |
+1 | mvnsite | 1m 32s | trunk passed |
+1 | shadedclient | 13m 7s | branch has no errors when building and testing our client artifacts. |
0 | findbugs | 0m 0s | Skipped patched modules with no Java source: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-site |
+1 | findbugs | 1m 23s | trunk passed |
+1 | javadoc | 1m 9s | trunk passed |
Patch Compile Tests | |||
0 | mvndep | 0m 12s | Maven dependency ordering for patch |
+1 | mvninstall | 1m 1s | the patch passed |
+1 | compile | 7m 32s | the patch passed |
+1 | javac | 7m 32s | the patch passed |
+1 | checkstyle | 1m 14s | the patch passed |
+1 | mvnsite | 1m 15s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | shadedclient | 10m 28s | patch has no errors when building and testing our client artifacts. |
0 | findbugs | 0m 0s | Skipped patched modules with no Java source: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-site |
+1 | findbugs | 1m 31s | the patch passed |
+1 | javadoc | 1m 3s | the patch passed |
Other Tests | |||
+1 | unit | 6m 46s | hadoop-yarn-services-core in the patch passed. |
+1 | unit | 0m 38s | hadoop-yarn-services-api in the patch passed. |
+1 | unit | 0m 20s | hadoop-yarn-site in the patch passed. |
+1 | asflicense | 0m 35s | The patch does not generate ASF License warnings. |
85m 12s |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8620d2b |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12918044/YARN-8126.001.patch |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle |
uname | Linux 47cba402b202 3.13.0-139-generic #188-Ubuntu SMP Tue Jan 9 14:43:09 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 5700556 |
maven | version: Apache Maven 3.3.9 |
Default Java | 1.8.0_151 |
findbugs | v3.1.0-RC1 |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/20269/testReport/ |
Max. process+thread count | 669 (vs. ulimit of 10000) |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-site U: hadoop-yarn-project/hadoop-yarn |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/20269/console |
Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
rohithsharma the patch looks good. Few minor comments -
SystemServiceManagerImpl.java
getbadDirSkipCounter make b in bad uppercase
Configurations.md
All service AM specific configs go here. If I understand correctly yarn.service.system-service.dir is a cluster-specific config, right?
Also, thanks for deleting TestSystemServiceManager.java which had all upgrade specific tests. I think I missed this in my first round review
If I understand correctly yarn.service.system-service.dir is a cluster-specific config, right?
Yes, How about adding new section Quick start?
I think this deserves to be a new sub-topic "System Services" on the left panel under "Service Discovery" (in the "YARN Service" section). It might seem that there is not much information for it to go to a new page, but there are primarily 2 reasons I am inclining towards it -
- An ordinary end-user cannot create or add services to be started as system-services. So it should not be in the existing pages which focuses on what ordinary end-users can do. Hence in this new page, we should specifically call out that this is a cluster admin feature.
- This is a pretty handy feature and going forward this page might grow as we add more system-service related features or add helpful system-services to the framework itself and would also need documentation to go with it.
What do you think?
It make sense to me to add new section though content may not be too much. As you said, it would grow down the line. I will update the patch with above document change.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 38s | 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 14 new or modified test files. |
trunk Compile Tests | |||
0 | mvndep | 0m 19s | Maven dependency ordering for branch |
+1 | mvninstall | 25m 46s | trunk passed |
+1 | compile | 28m 9s | trunk passed |
+1 | checkstyle | 3m 10s | trunk passed |
+1 | mvnsite | 2m 0s | trunk passed |
+1 | shadedclient | 16m 6s | branch has no errors when building and testing our client artifacts. |
0 | findbugs | 0m 0s | Skipped patched modules with no Java source: hadoop-project hadoop-yarn-project/hadoop-yarn/hadoop-yarn-site |
+1 | findbugs | 1m 20s | trunk passed |
+1 | javadoc | 1m 33s | trunk passed |
Patch Compile Tests | |||
0 | mvndep | 0m 17s | Maven dependency ordering for patch |
+1 | mvninstall | 1m 11s | the patch passed |
+1 | compile | 28m 2s | the patch passed |
+1 | javac | 28m 2s | the patch passed |
+1 | checkstyle | 3m 16s | the patch passed |
+1 | mvnsite | 1m 57s | the patch passed |
-1 | whitespace | 0m 0s | The patch has 3 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply |
+1 | xml | 0m 1s | The patch has no ill-formed XML file. |
+1 | shadedclient | 10m 19s | patch has no errors when building and testing our client artifacts. |
0 | findbugs | 0m 0s | Skipped patched modules with no Java source: hadoop-project hadoop-yarn-project/hadoop-yarn/hadoop-yarn-site |
+1 | findbugs | 1m 40s | the patch passed |
+1 | javadoc | 1m 29s | the patch passed |
Other Tests | |||
+1 | unit | 0m 22s | hadoop-project in the patch passed. |
+1 | unit | 6m 57s | hadoop-yarn-services-core in the patch passed. |
+1 | unit | 0m 43s | hadoop-yarn-services-api in the patch passed. |
+1 | unit | 0m 26s | hadoop-yarn-site in the patch passed. |
+1 | asflicense | 0m 45s | The patch does not generate ASF License warnings. |
133m 56s |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8620d2b |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12918731/YARN-8126.002.patch |
Optional Tests | asflicense mvnsite xml compile javac javadoc mvninstall unit shadedclient findbugs checkstyle |
uname | Linux ae540287f82d 3.13.0-139-generic #188-Ubuntu SMP Tue Jan 9 14:43:09 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 113af12 |
maven | version: Apache Maven 3.3.9 |
Default Java | 1.8.0_151 |
findbugs | v3.1.0-RC1 |
whitespace | https://builds.apache.org/job/PreCommit-YARN-Build/20323/artifact/out/whitespace-eol.txt |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/20323/testReport/ |
Max. process+thread count | 671 (vs. ulimit of 10000) |
modules | C: hadoop-project hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services/hadoop-yarn-services-core hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-site U: . |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/20323/console |
Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
rohithsharma patch 002 looks great. +1 to commit after you remove the trailing whitespaces from 3 lines in SystemServices.md.
Thanks gsaha. Reg the white space issue, this could be handled at the time of commit by using command git apply --whitespace=fix -v patch_file
sunilg Could you please help to commit this patch? And please use above command so that whitespace also will be fixed.
Copy pasting gsaha comments from
YARN-8048.1. SystemServiceManagerImpl.java
I think the info log will get printed here every time the warn log is also printed inside the if block. Should the info log go in the else block?
2. Should we rename TestSystemServiceImpl.java to TestSystemServiceManagerImpl.java?
3. TestSystemServiceImpl
Should we combine these 2 strings into 1 -> "Service name doesn't exist in expected " + "userService "
4. Do we need to add documentation about "yarn.service.system-service.dir” anywhere?
5. Under hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services-api/src/test/resources/ should we rename the dir “users" to "system-services" to better reflect what these files/tests are for (and change resourcePath in TestSystemServiceImpl.java accordingly)
6. For an additional test to see if a dir is skipped, do you want to add a directory named “bad" (probably needs a dummy file under it otherwise github will not allow you to commit an empty dir) under the path hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-services-api/src/test/resources//users/sync/user1/?