Details
-
Improvement
-
Status: Resolved
-
Major
-
Resolution: Fixed
-
2.8.0
-
None
-
Reviewed
Description
Attachments
Attachments
- YARN-4997-011.patch
- 23 kB
- Tao Jie
- YARN-4997-010.patch
- 23 kB
- Tao Jie
- YARN-4997-009.patch
- 23 kB
- Tao Jie
- YARN-4997-008.patch
- 23 kB
- Tao Jie
- YARN-4997-007.patch
- 23 kB
- Tao Jie
- YARN-4997-006.patch
- 23 kB
- Tao Jie
- YARN-4997-005.patch
- 23 kB
- Tao Jie
- YARN-4997-004.patch
- 23 kB
- Tao Jie
- YARN-4997-003.patch
- 23 kB
- Tao Jie
- YARN-4997-002.patch
- 22 kB
- Tao Jie
- YARN-4997-001.patch
- 22 kB
- Tao Jie
Issue Links
- is related to
-
YARN-6000 Make AllocationFileLoaderService.Listener public
- Resolved
- relates to
-
HIVE-15427 Hadoop3 support
- Open
Activity
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 11s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
0 | mvndep | 2m 47s | Maven dependency ordering for branch |
+1 | mvninstall | 8m 32s | trunk passed |
+1 | compile | 2m 55s | trunk passed |
+1 | checkstyle | 0m 46s | trunk passed |
+1 | mvnsite | 1m 26s | trunk passed |
+1 | mvneclipse | 0m 33s | trunk passed |
+1 | findbugs | 2m 19s | trunk passed |
+1 | javadoc | 0m 52s | trunk passed |
0 | mvndep | 0m 10s | Maven dependency ordering for patch |
+1 | mvninstall | 1m 9s | the patch passed |
+1 | compile | 2m 51s | the patch passed |
+1 | javac | 2m 51s | the patch passed |
-1 | checkstyle | 0m 44s | hadoop-yarn-project/hadoop-yarn: The patch generated 8 new + 321 unchanged - 3 fixed = 329 total (was 324) |
+1 | mvnsite | 1m 25s | the patch passed |
+1 | mvneclipse | 0m 31s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
-1 | findbugs | 1m 20s | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) |
+1 | javadoc | 0m 51s | the patch passed |
+1 | unit | 2m 35s | hadoop-yarn-common in the patch passed. |
-1 | unit | 35m 2s | hadoop-yarn-server-resourcemanager in the patch failed. |
+1 | asflicense | 0m 16s | The patch does not generate ASF License warnings. |
69m 29s |
Reason | Tests |
---|---|
FindBugs | module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common |
Incorrect lazy initialization of static field org.apache.hadoop.yarn.security.YarnAuthorizationProvider.authorizer in org.apache.hadoop.yarn.security.YarnAuthorizationProvider.destory() At YarnAuthorizationProvider.java:field org.apache.hadoop.yarn.security.YarnAuthorizationProvider.authorizer in org.apache.hadoop.yarn.security.YarnAuthorizationProvider.destory() At YarnAuthorizationProvider.java:[lines 65-67] | |
Failed junit tests | hadoop.yarn.server.resourcemanager.reservation.TestFairSchedulerPlanFollower |
hadoop.yarn.server.resourcemanager.scheduler.fair.TestFSLeafQueue |
This message was automatically generated.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 15s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 2 new or modified test files. |
0 | mvndep | 0m 9s | Maven dependency ordering for branch |
+1 | mvninstall | 6m 50s | trunk passed |
+1 | compile | 2m 21s | trunk passed |
+1 | checkstyle | 0m 41s | trunk passed |
+1 | mvnsite | 1m 10s | trunk passed |
+1 | mvneclipse | 0m 30s | trunk passed |
+1 | findbugs | 1m 54s | trunk passed |
+1 | javadoc | 0m 48s | trunk passed |
0 | mvndep | 0m 10s | Maven dependency ordering for patch |
+1 | mvninstall | 1m 0s | the patch passed |
+1 | compile | 2m 14s | the patch passed |
+1 | javac | 2m 14s | the patch passed |
-1 | checkstyle | 0m 46s | hadoop-yarn-project/hadoop-yarn: The patch generated 1 new + 334 unchanged - 3 fixed = 335 total (was 337) |
+1 | mvnsite | 1m 5s | the patch passed |
+1 | mvneclipse | 0m 26s | the patch passed |
-1 | whitespace | 0m 0s | The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix. |
-1 | findbugs | 1m 6s | 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 50s | the patch passed |
+1 | unit | 2m 17s | hadoop-yarn-common in the patch passed. |
+1 | unit | 33m 25s | hadoop-yarn-server-resourcemanager in the patch passed. |
+1 | asflicense | 0m 17s | The patch does not generate ASF License warnings. |
60m 8s |
Reason | Tests |
---|---|
FindBugs | module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager |
Inconsistent synchronization of org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.FairScheduler.authorizer; locked 50% of time Unsynchronized access at FairScheduler.java:50% of time Unsynchronized access at FairScheduler.java:[line 1602] |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:9560f25 |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12820898/YARN-4997-002.patch |
JIRA Issue | |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux 8a642c07ec10 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 204a205 |
Default Java | 1.8.0_101 |
findbugs | v3.0.0 |
checkstyle | https://builds.apache.org/job/PreCommit-YARN-Build/12559/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt |
whitespace | https://builds.apache.org/job/PreCommit-YARN-Build/12559/artifact/patchprocess/whitespace-eol.txt |
findbugs | https://builds.apache.org/job/PreCommit-YARN-Build/12559/artifact/patchprocess/new-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.html |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/12559/testReport/ |
modules | C: 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/12559/console |
Powered by | Apache Yetus 0.3.0 http://yetus.apache.org |
This message was automatically generated.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 16s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
0 | mvndep | 0m 10s | Maven dependency ordering for branch |
+1 | mvninstall | 7m 1s | trunk passed |
+1 | compile | 2m 25s | trunk passed |
+1 | checkstyle | 0m 42s | trunk passed |
+1 | mvnsite | 1m 11s | trunk passed |
+1 | mvneclipse | 0m 32s | trunk passed |
+1 | findbugs | 2m 5s | trunk passed |
+1 | javadoc | 0m 51s | trunk passed |
0 | mvndep | 0m 10s | Maven dependency ordering for patch |
+1 | mvninstall | 0m 57s | the patch passed |
+1 | compile | 2m 19s | the patch passed |
+1 | javac | 2m 19s | the patch passed |
-1 | checkstyle | 0m 40s | hadoop-yarn-project/hadoop-yarn: The patch generated 1 new + 322 unchanged - 3 fixed = 323 total (was 325) |
+1 | mvnsite | 1m 5s | the patch passed |
+1 | mvneclipse | 0m 26s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
-1 | findbugs | 1m 4s | 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 44s | the patch passed |
+1 | unit | 2m 14s | hadoop-yarn-common in the patch passed. |
+1 | unit | 36m 23s | hadoop-yarn-server-resourcemanager in the patch passed. |
+1 | asflicense | 0m 17s | The patch does not generate ASF License warnings. |
63m 25s |
Reason | Tests |
---|---|
FindBugs | module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager |
Inconsistent synchronization of org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.FairScheduler.authorizer; locked 50% of time Unsynchronized access at FairScheduler.java:50% of time Unsynchronized access at FairScheduler.java:[line 1602] |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:9560f25 |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12820905/YARN-4997-002.patch |
JIRA Issue | |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux 2fb4bc271934 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 204a205 |
Default Java | 1.8.0_101 |
findbugs | v3.0.0 |
checkstyle | https://builds.apache.org/job/PreCommit-YARN-Build/12560/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt |
findbugs | https://builds.apache.org/job/PreCommit-YARN-Build/12560/artifact/patchprocess/new-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.html |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/12560/testReport/ |
modules | C: 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/12560/console |
Powered by | Apache Yetus 0.3.0 http://yetus.apache.org |
This message was automatically generated.
Thanks for the patch, Tao Jie. Overall looks good to me. A couple of minor comments:
LOG.info(authorizer.getClass().getName() + " is destoryed.");
Probably best to make the log message at DEBUG level and only do the string concat if debug is on.
if (queueInfo == null) { authorizer.setPermission(allocsLoader.getDefaultPermissions(), UserGroupInformation.getCurrentUser()); return; }
Since that's in a small method, can we please use an else instead of returning from the if?
AccessControlList operationAcl = acls.get( SchedulerUtils.toAccessType(operation));
Tiny quibble... Can we split the line on the equals instead of on the paren?
authorizer .setPermission(permissions, UserGroupInformation.getCurrentUser());
Similarly, can we break on the comma here instead of the dot?
Finally, for the public and protected methods you added, please add javadocs.
templedf, thanks for your review! And attached patch with tiny fix associated with your comments.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 15s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
0 | mvndep | 0m 10s | Maven dependency ordering for branch |
+1 | mvninstall | 8m 11s | trunk passed |
+1 | compile | 2m 51s | trunk passed |
+1 | checkstyle | 0m 46s | trunk passed |
+1 | mvnsite | 1m 24s | trunk passed |
+1 | mvneclipse | 0m 32s | trunk passed |
+1 | findbugs | 2m 14s | trunk passed |
+1 | javadoc | 0m 52s | trunk passed |
0 | mvndep | 0m 12s | Maven dependency ordering for patch |
+1 | mvninstall | 1m 2s | the patch passed |
+1 | compile | 2m 43s | the patch passed |
+1 | javac | 2m 43s | the patch passed |
-1 | checkstyle | 0m 43s | hadoop-yarn-project/hadoop-yarn: The patch generated 3 new + 322 unchanged - 3 fixed = 325 total (was 325) |
+1 | mvnsite | 1m 14s | the patch passed |
+1 | mvneclipse | 0m 28s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
-1 | findbugs | 1m 18s | 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 20s | hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 2 new + 963 unchanged - 0 fixed = 965 total (was 963) |
-1 | unit | 2m 28s | hadoop-yarn-common in the patch failed. |
-1 | unit | 39m 9s | hadoop-yarn-server-resourcemanager in the patch failed. |
+1 | asflicense | 0m 17s | The patch does not generate ASF License warnings. |
69m 44s |
Reason | Tests |
---|---|
FindBugs | module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager |
Inconsistent synchronization of org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.FairScheduler.authorizer; locked 50% of time Unsynchronized access at FairScheduler.java:50% of time Unsynchronized access at FairScheduler.java:[line 1602] | |
Failed junit tests | hadoop.yarn.logaggregation.TestAggregatedLogFormat |
hadoop.yarn.server.resourcemanager.TestSubmitApplicationWithRMHA |
This message was automatically generated.
Thanks for the update. I still have a few nits to pick.
First, you're still missing some javadocs, and some of the javadocs you added is missing the @return tags.
In this code:
protected List<Permission> getDefaultPermissions() { if (defaultPermissions == null) { List<Permission> permissions = new ArrayList<Permission>(); Map<AccessType, AccessControlList> acls = new HashMap<AccessType, AccessControlList>(); for (QueueACL acl : QueueACL.values()) { acls.put(SchedulerUtils.toAccessType(acl), EVERYBODY_ACL); } permissions.add(new Permission( new PrivilegedEntity(EntityType.QUEUE, ROOT), acls)); defaultPermissions = permissions; } return defaultPermissions; }
Why the intermediate permissions variable? It smells like maybe an attempt at thread safety. If so, it's not enough. If not, can we drop the extra variable since it looks suspicious?
Here:
- public void onReload(AllocationConfiguration info); + void onReload(AllocationConfiguration info) throws IOException;
I'd rather we keep the public keyword. It's optional for interfaces, but it's easier to read with the keyword there.
templedf, thanks for reply! For getDefaultPermissions(), there is no reentrant problem since this method is only invoked by reloadAllocation(), which is already synchronized. The intermediate variable is used here is to avoid potential thread safety problem (Actually, it is not necessary) .
public keyword in onReload method here is removed since it trigger a checkstyle warning. It's ok to keep it.
Update the patch, please take a quick review!
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 18s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
0 | mvndep | 0m 11s | Maven dependency ordering for branch |
+1 | mvninstall | 7m 57s | trunk passed |
+1 | compile | 2m 18s | trunk passed |
+1 | checkstyle | 0m 41s | trunk passed |
+1 | mvnsite | 1m 7s | trunk passed |
+1 | mvneclipse | 0m 30s | trunk passed |
+1 | findbugs | 1m 52s | trunk passed |
+1 | javadoc | 0m 48s | trunk passed |
0 | mvndep | 0m 9s | Maven dependency ordering for patch |
+1 | mvninstall | 0m 56s | the patch passed |
+1 | compile | 2m 15s | the patch passed |
+1 | javac | 2m 15s | the patch passed |
-1 | checkstyle | 0m 39s | hadoop-yarn-project/hadoop-yarn: The patch generated 4 new + 322 unchanged - 3 fixed = 326 total (was 325) |
+1 | mvnsite | 1m 4s | the patch passed |
+1 | mvneclipse | 0m 25s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
-1 | findbugs | 1m 4s | 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 45s | the patch passed |
-1 | unit | 2m 15s | hadoop-yarn-common in the patch failed. |
+1 | unit | 38m 45s | hadoop-yarn-server-resourcemanager in the patch passed. |
+1 | asflicense | 0m 18s | The patch does not generate ASF License warnings. |
66m 5s |
Reason | Tests |
---|---|
FindBugs | module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager |
Inconsistent synchronization of org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.FairScheduler.authorizer; locked 50% of time Unsynchronized access at FairScheduler.java:50% of time Unsynchronized access at FairScheduler.java:[line 1602] | |
Failed junit tests | hadoop.yarn.logaggregation.TestAggregatedLogFormat |
This message was automatically generated.
Thanks for the updated patch, Tao Jie. Please see what you can do about resolving the first and last checkstyle issues and the findbugs issue. Also, if you don't mind, instead of adding more collections without the diamond operator, could you convert the neighboring collections over to using the diamond operator? (Did that make sense?)
Thanks for reply, templedf. I updated the code and found out that diamond operator is used since YARN-4207. It's fine to rebase the code and refine code style. I will update the patch soon, please review it again.
Thank you in advance
-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 appears to include 1 new or modified test files. |
0 | mvndep | 0m 9s | Maven dependency ordering for branch |
+1 | mvninstall | 6m 42s | trunk passed |
+1 | compile | 2m 19s | trunk passed |
+1 | checkstyle | 0m 42s | trunk passed |
+1 | mvnsite | 1m 8s | trunk passed |
+1 | mvneclipse | 0m 29s | trunk passed |
+1 | findbugs | 1m 50s | trunk passed |
+1 | javadoc | 0m 48s | trunk passed |
0 | mvndep | 0m 10s | Maven dependency ordering for patch |
+1 | mvninstall | 0m 56s | the patch passed |
+1 | compile | 2m 14s | the patch passed |
+1 | javac | 2m 14s | the patch passed |
-1 | checkstyle | 0m 39s | hadoop-yarn-project/hadoop-yarn: The patch generated 3 new + 315 unchanged - 3 fixed = 318 total (was 318) |
+1 | mvnsite | 1m 3s | the patch passed |
+1 | mvneclipse | 0m 26s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
-1 | findbugs | 1m 22s | 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 50s | the patch passed |
+1 | unit | 2m 32s | hadoop-yarn-common in the patch passed. |
-1 | unit | 38m 35s | hadoop-yarn-server-resourcemanager in the patch failed. |
+1 | asflicense | 0m 18s | The patch does not generate ASF License warnings. |
65m 31s |
Reason | Tests |
---|---|
FindBugs | module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager |
Inconsistent synchronization of org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.FairScheduler.authorizer; locked 50% of time Unsynchronized access at FairScheduler.java:50% of time Unsynchronized access at FairScheduler.java:[line 1604] | |
Failed junit tests | hadoop.yarn.server.resourcemanager.TestNodeBlacklistingOnAMFailures |
This message was automatically generated.
Thanks for the fresh patch. I looked more closely at the findbugs issue, and the problem is that setQueueAcls() isn't synchronized. Yes, it's only ever called from a synchronized context now, but no guarantee of that in the future. To prevent introducing issues later, it would be safest to also synchronize setQueueAcls(). The overhead will be minimal.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 12s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
0 | mvndep | 0m 58s | Maven dependency ordering for branch |
+1 | mvninstall | 7m 3s | trunk passed |
+1 | compile | 2m 20s | trunk passed |
+1 | checkstyle | 0m 41s | trunk passed |
+1 | mvnsite | 1m 8s | trunk passed |
+1 | mvneclipse | 0m 30s | trunk passed |
+1 | findbugs | 1m 56s | trunk passed |
+1 | javadoc | 0m 47s | trunk passed |
0 | mvndep | 0m 9s | Maven dependency ordering for patch |
+1 | mvninstall | 1m 0s | the patch passed |
+1 | compile | 2m 20s | the patch passed |
+1 | javac | 2m 20s | the patch passed |
-1 | checkstyle | 0m 39s | hadoop-yarn-project/hadoop-yarn: The patch generated 3 new + 314 unchanged - 3 fixed = 317 total (was 317) |
+1 | mvnsite | 1m 5s | the patch passed |
+1 | mvneclipse | 0m 26s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | findbugs | 2m 9s | the patch passed |
+1 | javadoc | 0m 43s | the patch passed |
+1 | unit | 2m 18s | hadoop-yarn-common in the patch passed. |
-1 | unit | 34m 4s | hadoop-yarn-server-resourcemanager in the patch failed. |
+1 | asflicense | 0m 18s | The patch does not generate ASF License warnings. |
61m 36s |
Reason | Tests |
---|---|
Failed junit tests | hadoop.yarn.server.resourcemanager.scheduler.capacity.TestNodeLabelContainerAllocation |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:9560f25 |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12825067/YARN-4997-006.patch |
JIRA Issue | |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux 0018f079314b 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 6f9c346 |
Default Java | 1.8.0_101 |
findbugs | v3.0.0 |
checkstyle | https://builds.apache.org/job/PreCommit-YARN-Build/12861/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt |
unit | https://builds.apache.org/job/PreCommit-YARN-Build/12861/artifact/patchprocess/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt |
unit test logs | https://builds.apache.org/job/PreCommit-YARN-Build/12861/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/12861/testReport/ |
modules | C: 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/12861/console |
Powered by | Apache Yetus 0.3.0 http://yetus.apache.org |
This message was automatically generated.
LGTM! Except... I apologize for not seeing this earlier. Your destroy() method is misspelled as destory().
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 18s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
0 | mvndep | 0m 26s | Maven dependency ordering for branch |
+1 | mvninstall | 7m 53s | trunk passed |
+1 | compile | 2m 26s | trunk passed |
+1 | checkstyle | 0m 42s | trunk passed |
+1 | mvnsite | 1m 11s | trunk passed |
+1 | mvneclipse | 0m 32s | trunk passed |
+1 | findbugs | 1m 58s | trunk passed |
+1 | javadoc | 0m 52s | trunk passed |
0 | mvndep | 0m 10s | Maven dependency ordering for patch |
+1 | mvninstall | 1m 3s | the patch passed |
+1 | compile | 2m 26s | the patch passed |
+1 | javac | 2m 26s | the patch passed |
-1 | checkstyle | 0m 41s | hadoop-yarn-project/hadoop-yarn: The patch generated 3 new + 315 unchanged - 3 fixed = 318 total (was 318) |
+1 | mvnsite | 1m 10s | the patch passed |
+1 | mvneclipse | 0m 28s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | findbugs | 2m 13s | the patch passed |
+1 | javadoc | 0m 48s | the patch passed |
+1 | unit | 2m 37s | hadoop-yarn-common in the patch passed. |
+1 | unit | 38m 15s | hadoop-yarn-server-resourcemanager in the patch passed. |
+1 | asflicense | 0m 17s | The patch does not generate ASF License warnings. |
67m 14s |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:9560f25 |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12825067/YARN-4997-006.patch |
JIRA Issue | |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux 3a17dda026cd 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | trunk / c37346d |
Default Java | 1.8.0_101 |
findbugs | v3.0.0 |
checkstyle | https://builds.apache.org/job/PreCommit-YARN-Build/12869/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/12869/testReport/ |
modules | C: 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/12869/console |
Powered by | Apache Yetus 0.3.0 http://yetus.apache.org |
This message was automatically generated.
Thanks for working on this, cassanada.
Few (mostly minor) comments on the latest patch:
- YarnAuthorizationProvider#destroy should be marked @VisibleForTesting.
- Noticed there is QueueACL is mapreduce code as well that can be dropped altogether? e.g. mapred QueueManager, many parts (all of?) QueueACL etc. Can we file a follow-up JIRA to drop all of that?
- AllocationFileLoaderService:
- getDefaultPermissions: don't need to specify type when creating an arraylist for defaultPermissions.
- Listener is an interface. Don't need to specify visibility - public?
- FairScheduler
- onReload: Is there a need to lock the scheduler when setting permissions? Would it be okay to limit the synchronized block to whatever was synchronized before?
- Similarly, is there a reason to synchronize setQueueAcls?
- In setQueueAcls, we seem to initially set to default permissions and then "overwrite" it with final permissions. Is the first one necessary? I quickly looked at implementation of ConfiguredAuthorizationProvider, setPermission's semantics appear to be somewhere between append and overwrite. If it is append, may be we should change that name to addPermission?
Thank you for your comments, kasha.
Noticed there is QueueACL is mapreduce code as well that can be dropped altogether? e.g. mapred QueueManager, many parts (all of?) QueueACL etc. Can we file a follow-up JIRA to drop all of that?
I am not sure if such code in mapred.QueueMananger still works today. I prefer to clean up those mapreduce code in another JIRA.
onReload: Is there a need to lock the scheduler when setting permissions? Would it be okay to limit the synchronized block to whatever was synchronized before?
Have disscussed with templedf, synchronized block added here is to avoid findbugs warning. Actually I will be glad to remove redundant lock here.
In setQueueAcls, we seem to initially set to default permissions and then "overwrite" it with final permissions. Is the first one necessary? I quickly looked at implementation of ConfiguredAuthorizationProvider, setPermission's semantics appear to be somewhere between append and overwrite. If it is append, may be we should change that name to addPermission?
I also feel a little confused about semantics of setPermission. However this abstract method is introduced by YARN-3100, and I'm not sure setPermission has implemented in Ranger or Sentry. I prefer to keep setPermission here as CapacityScheduler does to keep compatibility. Maybe we could refactor it in another JIRA, (maybe could separate setPermission to setPermission, addPermission, removePermission, clearPermission). Does it make sense?
And I will update this patch soon.
I looked more closely at synchronized in onReload. onReload here is under the lock of AllocationFileLoaderService, but initialization of authorizer is under the lock of FairScheduler. As a result, we'd better to keep all access to authorizer under the same lock of FairScheduler(Actually, initScheduler and reload won't happen at the same time, but they are called in different threads).
Hi,everyOne
this issue is very good,when this patch release?
I want to test in the ranger with the new yarn version.
Hi kasha, I rebased the patch for review.
For the semantics of setPermission, I checked code in RANGER, where RangerYarnAuthorizer extends YarnAuthorizationProvider and override method setPermission. As a result, we should keep this method as it used to be.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 26s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
0 | mvndep | 0m 12s | Maven dependency ordering for branch |
+1 | mvninstall | 9m 6s | trunk passed |
+1 | compile | 3m 5s | trunk passed |
+1 | checkstyle | 0m 52s | trunk passed |
+1 | mvnsite | 1m 33s | trunk passed |
+1 | mvneclipse | 0m 42s | trunk passed |
+1 | findbugs | 2m 16s | trunk passed |
+1 | javadoc | 1m 10s | trunk passed |
0 | mvndep | 0m 14s | Maven dependency ordering for patch |
+1 | mvninstall | 1m 16s | the patch passed |
+1 | compile | 2m 58s | the patch passed |
+1 | javac | 2m 58s | the patch passed |
-0 | checkstyle | 0m 40s | hadoop-yarn-project/hadoop-yarn: The patch generated 2 new + 300 unchanged - 4 fixed = 302 total (was 304) |
+1 | mvnsite | 1m 9s | the patch passed |
+1 | mvneclipse | 0m 29s | the patch passed |
-1 | whitespace | 0m 0s | The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply |
+1 | findbugs | 2m 22s | the patch passed |
+1 | javadoc | 0m 51s | the patch passed |
+1 | unit | 2m 23s | hadoop-yarn-common in the patch passed. |
+1 | unit | 39m 30s | hadoop-yarn-server-resourcemanager in the patch passed. |
+1 | asflicense | 0m 20s | The patch does not generate ASF License warnings. |
79m 26s |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:9560f25 |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12836308/YARN-4997-009.patch |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux 00a0e48d85a0 3.13.0-96-generic #143-Ubuntu SMP Mon Aug 29 20:15:20 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 7ba74be |
Default Java | 1.8.0_101 |
findbugs | v3.0.0 |
checkstyle | https://builds.apache.org/job/PreCommit-YARN-Build/13724/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt |
whitespace | https://builds.apache.org/job/PreCommit-YARN-Build/13724/artifact/patchprocess/whitespace-eol.txt |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/13724/testReport/ |
modules | C: 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/13724/console |
Powered by | Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
+1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 15s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
0 | mvndep | 0m 9s | Maven dependency ordering for branch |
+1 | mvninstall | 7m 28s | trunk passed |
+1 | compile | 2m 18s | trunk passed |
+1 | checkstyle | 0m 41s | trunk passed |
+1 | mvnsite | 1m 7s | trunk passed |
+1 | mvneclipse | 0m 30s | trunk passed |
+1 | findbugs | 1m 51s | trunk passed |
+1 | javadoc | 0m 54s | trunk passed |
0 | mvndep | 0m 10s | Maven dependency ordering for patch |
+1 | mvninstall | 1m 0s | the patch passed |
+1 | compile | 2m 26s | the patch passed |
+1 | javac | 2m 26s | the patch passed |
-0 | checkstyle | 0m 42s | hadoop-yarn-project/hadoop-yarn: The patch generated 2 new + 300 unchanged - 4 fixed = 302 total (was 304) |
+1 | mvnsite | 1m 12s | the patch passed |
+1 | mvneclipse | 0m 30s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | findbugs | 2m 16s | the patch passed |
+1 | javadoc | 0m 47s | the patch passed |
+1 | unit | 2m 18s | hadoop-yarn-common in the patch passed. |
+1 | unit | 35m 3s | hadoop-yarn-server-resourcemanager in the patch passed. |
+1 | asflicense | 0m 19s | The patch does not generate ASF License warnings. |
69m 33s |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:9560f25 |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12836328/YARN-4997-009.patch |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux cf2f37136b55 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 / 7d2d8d2 |
Default Java | 1.8.0_101 |
findbugs | v3.0.0 |
checkstyle | https://builds.apache.org/job/PreCommit-YARN-Build/13730/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/13730/testReport/ |
modules | C: 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/13730/console |
Powered by | Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
Thanks for updating the patch. Since kasha is out for a little while, I'm jumping back in. Looks like you (pl.) decided to drop the synchronized and screw the checkstyle complaint. In the interest of not going in circles, I can live with that. Other minor nits:
- Can we have AllocationConfiguration.getQueueAcls() wrap the Map in a Collections.unmodifiableMap()? It makes me a little nervous to expose mutable data structures in getters.
- The javadoc for AllocationFileLoaderService. getDefaultPermissions() should start with a summary sentence that ends with a period. Aside from not being a good summary, the current first sentence is missing the period.
- In FairScheduler, you messed up the indentation of the first line of applyChildDefaults()'s javadoc.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 15s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
0 | mvndep | 0m 9s | Maven dependency ordering for branch |
+1 | mvninstall | 6m 55s | trunk passed |
+1 | compile | 5m 23s | trunk passed |
+1 | checkstyle | 0m 49s | trunk passed |
+1 | mvnsite | 1m 25s | trunk passed |
+1 | mvneclipse | 0m 43s | trunk passed |
+1 | findbugs | 2m 6s | trunk passed |
+1 | javadoc | 1m 0s | trunk passed |
0 | mvndep | 0m 10s | Maven dependency ordering for patch |
+1 | mvninstall | 0m 58s | the patch passed |
+1 | compile | 4m 40s | the patch passed |
+1 | javac | 4m 40s | the patch passed |
-0 | checkstyle | 0m 45s | hadoop-yarn-project/hadoop-yarn: The patch generated 2 new + 289 unchanged - 4 fixed = 291 total (was 293) |
+1 | mvnsite | 1m 18s | the patch passed |
+1 | mvneclipse | 0m 40s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | findbugs | 2m 19s | the patch passed |
+1 | javadoc | 0m 59s | the patch passed |
+1 | unit | 2m 24s | hadoop-yarn-common in the patch passed. |
-1 | unit | 42m 28s | hadoop-yarn-server-resourcemanager in the patch failed. |
+1 | asflicense | 0m 29s | The patch does not generate ASF License warnings. |
84m 6s |
Reason | Tests |
---|---|
Failed junit tests | hadoop.yarn.server.resourcemanager.TestRMRestart |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:a9ad5d6 |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12840787/YARN-4997-010.patch |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux b4e143324bcb 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 / 47ca9e2 |
Default Java | 1.8.0_111 |
findbugs | v3.0.0 |
checkstyle | https://builds.apache.org/job/PreCommit-YARN-Build/14093/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt |
unit | https://builds.apache.org/job/PreCommit-YARN-Build/14093/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/14093/testReport/ |
modules | C: 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/14093/console |
Powered by | Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
Updated the patch respect to templedf's comment. The test failure is irrelevant.
Thanks, Tao Jie. Sorry to be fussy, but can we add a first line to the AllocationFileLoaderService.getDefaultPermissions() javadoc, something like, "Returns the list of default permissions." Javadoc takes the first sentence as the summary that is shown in the table of methods.
templedf, thanks for your patient review and sorry for inaccurate understanding of your comments.
Updated the patch..
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 16s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
0 | mvndep | 0m 10s | Maven dependency ordering for branch |
+1 | mvninstall | 7m 1s | trunk passed |
+1 | compile | 4m 58s | trunk passed |
+1 | checkstyle | 0m 51s | trunk passed |
+1 | mvnsite | 1m 33s | trunk passed |
+1 | mvneclipse | 0m 43s | trunk passed |
+1 | findbugs | 2m 23s | trunk passed |
+1 | javadoc | 1m 7s | trunk passed |
0 | mvndep | 0m 11s | Maven dependency ordering for patch |
+1 | mvninstall | 1m 1s | the patch passed |
+1 | compile | 4m 50s | the patch passed |
+1 | javac | 4m 50s | the patch passed |
-0 | checkstyle | 0m 49s | hadoop-yarn-project/hadoop-yarn: The patch generated 2 new + 287 unchanged - 4 fixed = 289 total (was 291) |
+1 | mvnsite | 1m 28s | the patch passed |
+1 | mvneclipse | 0m 44s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | findbugs | 2m 41s | the patch passed |
+1 | javadoc | 1m 2s | the patch passed |
+1 | unit | 2m 33s | hadoop-yarn-common in the patch passed. |
-1 | unit | 39m 50s | hadoop-yarn-server-resourcemanager in the patch failed. |
+1 | asflicense | 0m 29s | The patch does not generate ASF License warnings. |
82m 46s |
Reason | Tests |
---|---|
Failed junit tests | hadoop.yarn.server.resourcemanager.TestRMRestart |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:a9ad5d6 |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12840995/YARN-4997-011.patch |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux 81a82c6e2ca7 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | trunk / aeecfa2 |
Default Java | 1.8.0_111 |
findbugs | v3.0.0 |
checkstyle | https://builds.apache.org/job/PreCommit-YARN-Build/14116/artifact/patchprocess/diff-checkstyle-hadoop-yarn-project_hadoop-yarn.txt |
unit | https://builds.apache.org/job/PreCommit-YARN-Build/14116/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/14116/testReport/ |
modules | C: 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/14116/console |
Powered by | Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
I'll let you slide on the method length checkstyle complaints, and it looks like the test failure is unrelated. +1
Tao Jie, would you mind checking if the change you made in the FairScheduler test needs to be made in any other tests? If so, please file a new JIRA.
SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10915 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10915/)
YARN-4997. Update fair scheduler to use pluggable auth provider (templedf: rev b3befc021b0e2d63d1a3710ea450797d1129f1f5)
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/security/YarnAuthorizationProvider.java
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FSQueue.java
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FairScheduler.java
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/TestFairScheduler.java
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/AllocationFileLoaderService.java
this break the following piece of code in Hive that seems to be determining the default queue (I am not familiar with all the APIs called here), because the listener interface is made invisible while the setReloadListener API that it is passed into is still public.
AllocationFileLoaderService allocsLoader = new AllocationFileLoaderService(); allocsLoader.init(conf); allocsLoader.setReloadListener(new AllocationFileLoaderService.Listener() { @Override public void onReload(AllocationConfiguration allocs) { allocConf.set(allocs); } }); try { allocsLoader.reloadAllocations(); } catch (Exception ex) { throw new IOException("Failed to load queue allocations", ex); } if (allocConf.get() == null) { allocConf.set(new AllocationConfiguration(conf)); } QueuePlacementPolicy queuePolicy = allocConf.get().getPlacementPolicy(); if (queuePolicy != null) { requestedQueue = queuePolicy.assignAppToQueue(requestedQueue, userName);
Can you recommend a way to utilize reloadAllocations and get the queue (or placement policy, or allocConf) without invoking this? Or some other workaround.
Or otherwise can you please change Listener back to public?
Checking the code in 2.6, it seems like we can getConfig from the loader, but whatever reloadAllocations might derive from the placement policy element in the xml file is not accessible.
sershe, we have discussed about the modifier of interface Listener earlier in this patch. We removed public on interface Listener since it got a findbugs warning, and found public here is not necessary.
Since this breaks Hive code, I prefer to add public back to Listener.
We'd be ok with a different way; our existing code, as is, seems very convoluted to me. All we need is to get the correct placement policy (presumably, based on both config and the xml file that reloadAllocations uses).
Thank you sershe, I have created another JIRA YARN-6000 to handle this.
It's OK if you change Hive code to walk around and make the logic more clear. However once it break the code, we should get it fixed. Otherwise, when we update the Hadoop version, but not Hive(maybe have not released yet), it would fail.
hi templedf, I would like to take this over, if you haven't started work on this.