- 
                Notifications
    You must be signed in to change notification settings 
- Fork 9.1k
YARN-11263. [JDK17] Upgrade JUnit from 4 to 5 in hadoop-yarn-server-nodemanager Part1. #7390
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
| 💔 -1 overall 
 
 This message was automatically generated. | 
| 💔 -1 overall 
 
 This message was automatically generated. | 
ad1aa12    to
    1364afe      
    Compare
  
    | 💔 -1 overall 
 
 
 This message was automatically generated. | 
f3fa30e    to
    b7eb2c9      
    Compare
  
    | 💔 -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. | 
| @cnauroth Could you please help review this PR? Thank you very much! | 
| 🎊 +1 overall 
 
 This message was automatically generated. | 
| public boolean https; | ||
|  | ||
| @Before | ||
| public void initHttps(boolean pHttps) { | 
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.
Can this be made private?
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 reviewing the code! I will also pay attention to similar issues in other classes and make improvements.
| public boolean https; | ||
|  | ||
| @Before | ||
| public void initHttps(boolean pHttps) throws ContainerExecutionException { | 
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.
Can this be made private?
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 fix this issue.
| 🎊 +1 overall 
 
 This message was automatically generated. | 
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.
+1. Thank you again, @slfan1989 .
| @cnauroth Thank you very much for reviewing the code! I have merged it into the trunk branch to start the JUnit5 upgrade for NodeManager Part2. | 
…odemanager Part1. (apache#7390) [JDK17] Upgrade JUnit from 4 to 5 in hadoop-yarn-server-nodemanager. Co-authored-by: Chris Nauroth <[email protected]> Reviewed-by: Chris Nauroth <[email protected]> Signed-off-by: Shilun Fan <[email protected]>
Description of PR
JIRA: YARN-11263. [JDK17] Upgrade JUnit from 4 to 5 in hadoop-yarn-server-nodemanager.
How was this patch tested?
Junit & mvn clean test.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?