-
Notifications
You must be signed in to change notification settings - Fork 9.1k
YARN-11374. [Federation] Support refreshSuperUserGroupsConfiguration、refreshUserToGroupsMappings API's for Federation. #5193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…refreshUserToGroupsMappings API's for Federation.
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
return false; | ||
} | ||
if (other.getClass().isAssignableFrom(this.getClass())) { | ||
return this.getProto().equals(this.getClass().cast(other).getProto()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a cleaner way to do all this check or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for helping to review the code, I will modify the code.
} | ||
|
||
routerMetrics.incrRefreshQueuesFailedRetrieved(); | ||
throw new YarnException("Unable to refreshQueue."); | ||
throw new YarnException("Unable to refreshQueue due to exception."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to change this?
@@ -192,11 +192,11 @@ public RefreshQueuesResponse refreshQueues(RefreshQueuesRequest request) | |||
} | |||
} catch (YarnException e) { | |||
routerMetrics.incrRefreshQueuesFailedRetrieved(); | |||
RouterServerUtil.logAndThrowException(e, "Unable to refreshQueue due to exception."); | |||
throw e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No log anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that if Unable to refreshUserToGroupsMappings due to exception.
is directly displayed, the user still does not know why the exception occurred. It is better to display like this, SC-2 is not an active subCluster.
, so I choose to throw the exception directly.
Stack information before modification
Caused by: org.apache.hadoop.yarn.exceptions.YarnException: Unable to refreshUserToGroupsMappings due to exception.
at org.apache.hadoop.yarn.server.router.RouterServerUtil.logAndThrowException(RouterServerUtil.java:82)
at org.apache.hadoop.yarn.server.router.rmadmin.FederationRMAdminInterceptor.refreshUserToGroupsMappings(FederationRMAdminInterceptor.java:362)
at org.apache.hadoop.yarn.server.router.rmadmin.TestFederationRMAdminInterceptor.lambda$testSC1RefreshUserToGroupsMappings$7(TestFederationRMAdminInterceptor.java:260)
at org.apache.hadoop.test.LambdaTestUtils.intercept(LambdaTestUtils.java:498)
at org.apache.hadoop.test.LambdaTestUtils.intercept(LambdaTestUtils.java:384)
at org.apache.hadoop.test.LambdaTestUtils.intercept(LambdaTestUtils.java:453)
... 30 more
Caused by: org.apache.hadoop.yarn.exceptions.YarnException: subClusterId = SC-2 is not an active subCluster.
at org.apache.hadoop.yarn.server.router.rmadmin.RMAdminProtocolMethod.invoke(RMAdminProtocolMethod.java:165)
at org.apache.hadoop.yarn.server.router.rmadmin.RMAdminProtocolMethod.invokeConcurrent(RMAdminProtocolMethod.java:69)
at org.apache.hadoop.yarn.server.router.rmadmin.FederationRMAdminInterceptor.refreshUserToGroupsMappings(FederationRMAdminInterceptor.java:352)
... 34 more
I agree with you that we should log, so I'm going to modify it like this.
org.apache.hadoop.yarn.exceptions.YarnException: Unable to refreshUserToGroupsMappings due to exception. subClusterId = SC-2 is not an active subCluster.
at org.apache.hadoop.yarn.server.router.RouterServerUtil.logAndThrowException(RouterServerUtil.java:82)
at org.apache.hadoop.yarn.server.router.rmadmin.FederationRMAdminInterceptor.refreshUserToGroupsMappings(FederationRMAdminInterceptor.java:362)
at org.apache.hadoop.yarn.server.router.rmadmin.TestFederationRMAdminInterceptor.testSC1RefreshUserToGroupsMappings(TestFederationRMAdminInterceptor.java:258)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69)
at com.intellij.rt.junit.IdeaTestRunner$Repeater$1.execute(IdeaTestRunner.java:38)
at com.intellij.rt.execution.junit.TestsRepeater.repeat(TestsRepeater.java:11)
at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:35)
at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:235)
at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:54)
Caused by: org.apache.hadoop.yarn.exceptions.YarnException: subClusterId = SC-NON is not an active subCluster.
at org.apache.hadoop.yarn.server.router.rmadmin.RMAdminProtocolMethod.invoke(RMAdminProtocolMethod.java:165)
at org.apache.hadoop.yarn.server.router.rmadmin.RMAdminProtocolMethod.invokeConcurrent(RMAdminProtocolMethod.java:69)
at org.apache.hadoop.yarn.server.router.rmadmin.FederationRMAdminInterceptor.refreshUserToGroupsMappings(FederationRMAdminInterceptor.java:352)
... 30 more
...arn/server/api/protocolrecords/impl/pb/RefreshSuperUserGroupsConfigurationRequestPBImpl.java
Show resolved
Hide resolved
@Override | ||
public String getSubClusterId() { | ||
RefreshSuperUserGroupsConfigurationRequestProtoOrBuilder p = viaProto ? proto : builder; | ||
return (p.hasSubClusterId()) ? p.getSubClusterId() : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to expand the ternary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will modify the code.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@goiri Can you help review this pr? Thank you very much! |
💔 -1 overall
This message was automatically generated. |
@goiri Can you help to merge this pr into the trunk branch? Thank you very much! The unit test reports an error, not caused by our code. I will continue to follow up YARN-11375. |
Let's get YARN-11413 in first. |
🎊 +1 overall
This message was automatically generated. |
@goiri Thank you very much for helping to review the code! |
JIRA: YARN-11374. [Federation] Support refreshSuperUserGroupsConfiguration、refreshUserToGroupsMappings API's for Federation.