[ https://issues.apache.org/jira/browse/YARN-10003?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17052141#comment-17052141 ]
Benjamin Teke edited comment on YARN-10003 at 3/5/20, 1:39 PM: --------------------------------------------------------------- Hi [~adam.antal], Thanks for the review, makes sense, updated the patch based on the comments. About this: - I don't see why we catch the exception in {{TestInMemoryConfigurationStore#checkVersion}} - if an exception is thrown test is failed anyways, and the {{Assert#fail}} method does not give further information. This fail serves only the purpose of easy understanding. Based on a best practice ([https://pmd.github.io/latest/pmd_rules_java_bestpractices.html#junittestsshouldincludeassert]) it shows that the unittest is in fact testing that the method does not throw any exception. But if it's unnecessary I could remove it, of course. was (Author: bteke): Hi [~adam.antal], Thanks for the review, makes sense, updated the patch based on the comments. About this: - I don't see why we catch the exception in {{TestInMemoryConfigurationStore#checkVersion}} - if an exception is thrown test is failed anyways, and the {{Assert#fail}} method does not give further information. This fail serves only the purpose of easy understanding. Based on a best practice ([https://pmd.github.io/latest/pmd_rules_java_bestpractices.html#junittestsshouldincludeassert]) it shows that the unittest is testing that the method does not throw any exception. But if it's unnecessary I could remove it, of course. > YarnConfigurationStore#checkVersion throws exception that belongs to > RMStateStore > --------------------------------------------------------------------------------- > > Key: YARN-10003 > URL: https://issues.apache.org/jira/browse/YARN-10003 > Project: Hadoop YARN > Issue Type: Improvement > Reporter: Szilard Nemeth > Assignee: Benjamin Teke > Priority: Major > Attachments: YARN-10003.001.patch, YARN-10003.002.patch, > YARN-10003.003.patch, YARN-10003.004.patch > > > RMStateVersionIncompatibleException is thrown from method "checkVersion". > Moreover, there's a TODO here saying this method is copied from RMStateStore. > We should revise this method a bit. -- This message was sent by Atlassian Jira (v8.3.4#803005) --------------------------------------------------------------------- To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org