[ https://issues.apache.org/jira/browse/YARN-9270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16766185#comment-16766185 ]
Adam Antal commented on YARN-9270: ---------------------------------- Thanks for the patch, [~pbacsko]! Some thought of mine: - If we touch {{IntelFpgaOpenclPlugin.java}}, could we remove the wildcard importĀ {{import java.util.*}}. If I'm not mistaken, we use HashMap, LinkedList, List and Map in that file. (similar to TestFpgaDiscoverer.java). - Removing that dirty hack from {{TestFpgaDiscoverer.java}} is a great plus of this patch, thank you for that! I find the exact same piece of code in SO by searching the keywords (it's a red flag for me), and it's looks really messy, so I am happy if we can get this removed. - I don't see why the constructor of Configuration is called with false, but I can accept that. Also the 5th testcase (testLinuxFpgaResourceDiscoverPluginWithSdkRootSet) uses another Conifiguration object in the original testcase when calling the {{discoverer.initialize(conf)}} (which is initialized with a true parameter) - so you modify the behaviour of the testcase. It doesn't make the test fail, but is it intentional? - We request the instance of the FpgaDiscoverer 5 times, and then call the setResourceHanderPlugin on it with the same parameter (openclPlugin). Can we move it to a helper function to avoid minor code duplication? - We can also move the setting of YarnConfiguration.NM_FPGA_PATH_TO_EXEC config to that function, if we don't modify the 1st test behaviour. - Also could you move the previous comments/description of the test cases to the new tests' javadoc? - As I see it, there aren't any logs defined in this testcase. It is beyond the scope of the issue, but it would be nice to have some debug level logging. For a start it'd be nice to have log for the new tests that you just split. Was happy to review it, good work overall! > Minor cleanup in TestFpgaDiscoverer > ----------------------------------- > > Key: YARN-9270 > URL: https://issues.apache.org/jira/browse/YARN-9270 > Project: Hadoop YARN > Issue Type: Sub-task > Reporter: Peter Bacsko > Assignee: Peter Bacsko > Priority: Major > Attachments: YARN-9270-001.patch > > > Let's do some cleanup in this class. > * {{testLinuxFpgaResourceDiscoverPluginConfig}} - this test should be split > up to 5 different tests, because it tests 5 different scenarios. > * remove {{setNewEnvironmentHack()}} - too complicated. We can introduce a > {{Function}} in the plugin class like {{Function<String, String> envProvider > = System::getenv()}} plus a setter method which allows the test to modify > {{envProvider}}. Much simpler and straightfoward. -- This message was sent by Atlassian JIRA (v7.6.3#76005) --------------------------------------------------------------------- To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org