szilard-nemeth commented on PR #5119: URL: https://github.com/apache/hadoop/pull/5119#issuecomment-1374763585
Hi @susheel-gupta Thanks for working on this. 1. This should be a fairly simple and concise PR. Now we are having 66 changed files. I don't see a reason to move waitForState from MockRM and MockAM. Especially now that ResourceManagerTesterUtil and MockRM / MockAM has the logic scattered. Before the patch, every related logic were in the MockRM / MockAM classes. The description of the jira ticket says that the waitForContainerCompletion method of TestContainerResourceUsage should be moved to a utility class or to a mock class. In this case, MockRM would be a perfect place to move it as this class has similar methods already so the PR would be far more simple. So I don't think creating ResourceManagerTesterUtil has any added value. 2. In fact, the original method in org.apache.hadoop.yarn.server.resourcemanager.MockRM#waitForState is also kept without any remaining usages so it should be removed. 3. It's not a good practice to mix a library upgrade / update (in this case JUnit) with other code changes. I understand that the tests were failing (as per this comment: https://github.com/apache/hadoop/pull/5119#issuecomment-1330105691). The JUnit stuff has to go in before this patch, with a separate jira. Please file another jira for the JUnit upgrade. You can mention that what was the reason to use the new version of it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
