hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam Antal (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-9270) Minor cleanup in TestFpgaDiscoverer
Date Tue, 12 Feb 2019 16:01:00 GMT

    [ 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-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message