[
https://issues.apache.org/jira/browse/YARN-9266?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16768375#comment-16768375
]
Adam Antal commented on YARN-9266:
----------------------------------
Thanks for the patch, [~pbacsko]! It looks good overall, and hope this can be
committed soon as other issues are depending on it.
{panel:title=General comments}
- Some of the refactors that you performed are not included in the description
of the jira. Could you update it? I'm thinking of for example moving the parser
to separate file and fixing checkstyles.
- Actually fixing checkstyles in general should be avoided as it's making the
backports harder and making the git history dirtier. If you could static import
the assertTrue/False/Equals functions, replace the
assert.AssertTrue/False/Equals ones and ALSO fixing checkstyles, I can get away
with that.
{panel}
{panel:title=FpgaResourceHandlerImpl.java}
- Can we move the comments in function preStart to a javadoc?
{panel}
{panel:title=TestFpgaResourceHandler.java}
- Wildcard imports:
-- {{java.util.*}} -> only used List, ArrayList, Map, HashMap.
-- From {{org.mockito.mockito}} 10 different class is used: mock, when, verify,
times, anyString, anyMap, anyList, never, eq, atLeastOnce. As I see it in other
files, this is acceptable to use the wildcard character here.
-- From {{org.apache.hadoop.yarn.api.records}} we use 6 classes. It is
acceptable to have a wildcard character here as well, but separate imports
would look nicer. (Classes: ContainerId, ResourceInformation, Resource,
ContainerLaunchContext, ApplicationAttemptId, ApplicationId)
- There's an extra space before import {{java.io.IOException}}
- In this file we could also static import assert.
{panel}
{panel:title=AoclDiagnosticOutputParser.java}
- In line 105 can we replace the null and section in the following condition:
{code:java}
if (section == null)
{code}
- Can we make parseDiagnosticOutput function static? Thus we can avoid
constructing an empty, stateless parser object each time we want to parse the
output of the diagnostic.
- For sake of completeness, I suggest adding some logging there. In case
something fails, we can get some partial look into the parser by the logging
(I'm thinking of situations like we could parse 2 parts of the diagnostic
output and failing at the 3rd). It can make debugging and fixing the parser
easier, if the diagnostic output changes, which is a potential problem. I'm not
forcing to do that in this jira, maybe we can add this to YARN-9304 after the
issue has been committed.
{panel}
{panel:title=IntelFpgaOpenclPlugin.java}
- {{java.util.*}}
- Do we need the conf Configuration object of {{AbstractFpgaVendorPlugin}} at
all? It is not used anywhere, and we're using Configuration object only in
initPlugin where we use the parameter, and the class variable.
- In line 171: as I see we do not use the variable e in {{catch (IOException
e)}}? If it is important, let's log it, if not, then replace "e" with "ignore"
indicating we do not intend to use it.
- In line 172: no need to keep the String msg variable, we just have to log it.
{panel}
> Various fixes are needed in IntelFpgaOpenclPlugin
> -------------------------------------------------
>
> Key: YARN-9266
> URL: https://issues.apache.org/jira/browse/YARN-9266
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Peter Bacsko
> Assignee: Peter Bacsko
> Priority: Major
> Attachments: YARN-9266-001.patch, YARN-9266-002.patch,
> YARN-9266-003.patch, YARN-9266-004.patch
>
>
> Problems identified in this class:
> * {{InnerShellExecutor}} ignores the timeout parameter
> * {{configureIP()}} uses printStackTrace() instead of logging
> * {{configureIP()}} does not log the output of aocl if the exit code != 0
> * {{parseDiagnoseInfo()}} is too heavyweight – it should be in its own class
> for better testability
> * {{downloadIP()}} uses {{contains()}} for file name check – this can really
> surprise users in some cases (eg. you want to use hello.aocx but hello2.aocx
> also matches)
> * method name {{downloadIP()}} is misleading – it actually tries to finds
> the file. Everything is downloaded (localized) at this point.
> * {{@VisibleForTesting}} methods should be package private
> * {{aliasMap}} is not needed - store the acl number in the {{FpgaDevice}}
> class
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
