-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
YARN-6946. Upgrade JUnit from 4 to 5 in hadoop-yarn-common #4717
Conversation
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@ashutoshcipher Thank you very much for your contribution, it is very useful. can you submit it as a subpackage, I feel that the changes are a bit big. |
@slfan1989 - Thanks for review and approval. I think its overall one package and we keep it as it. The changes seems be kinda okay not really huge :) May be in further modules - I will try to keep short. Again, Thanks for suggestion. |
@aajisaka - Please help in reviewing the PR. Thanks. |
1 similar comment
@aajisaka - Please help in reviewing the PR. Thanks. |
@aajisaka Please help in reviewing the PR. Thank a lot. |
Thank you @ashutoshcipher for your patch. I reviewed 57 out of 105 files and now I have no comments. I'll review the rest tomorrow. |
Thank you so much @aajisaka |
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 @ashutoshcipher for completing the upgrades in YARN
I want to check if there's other YARN modules depend on the test class in hadoop-yarn-common before merging this. |
Will trigger jenkins by adding a temp empty line in |
02c6f63
to
b66218c
Compare
b66218c
to
2778068
Compare
@aajisaka - I am not able trigger jenkins. I dont see any merge conflict as well. Can you please have a look? Thanks. |
I could reproduce it. It's related to YARN-11294. Would you rebase it for the latest trunk? I'm not sure why GitHub doesn't complain about it. |
Thanks @aajisaka, will do that. |
💔 -1 overall
This message was automatically generated. |
3b21d45
to
f570120
Compare
🎊 +1 overall
This message was automatically generated. |
@aajisaka - Results looks good to me. Can you also check once? If it looks good to you, I can revert the empty line changes made in this file - Thanks. |
Thank you @ashutoshcipher The test result looks good to me. Could you revert the empty line change? |
Thanks @aajisaka I will revert that |
@aajisaka - Done in my last commit. |
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 @ashutoshcipher for your contribution and thank you @slfan1989 for your review! Also, thank you for cleaning up bunch of checkstyle issues :)
|
Thanks @aajisaka for reviewing and merging. |
🎊 +1 overall
This message was automatically generated. |
Co-authored-by: Ashutosh Gupta <[email protected]> Signed-off-by: Akira Ajisaka <[email protected]>
Description of PR
Upgrade JUnit from 4 to 5 in hadoop-yarn-common
JIRA - YARN-6946
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?