[
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: [email protected]
For additional commands, e-mail: [email protected]
