[
https://issues.apache.org/jira/browse/YARN-9266?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16768648#comment-16768648
]
Peter Bacsko commented on YARN-9266:
------------------------------------
"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."
I added checkstyle. Moving the parser is mentioned, albeit not explicitly
("parseDiagnoseInfo() is too heavyweight – it should be in its own class for
better testability")
"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."
This is generally true, however, FPGA is a bit different IMO.
1. It's still considered beta, not really used by anyone
2. The code is relatively new
3. Changes are very isolated
4. It will likely be deprecated by the pluggable device framework (not sure
when)
Having said that, I can revert the checkstyle changes. I'd rather keep it and
focus on static importing the asserts though.
"Can we move the comments in function preStart to a javadoc?"
Let's do this in YARN-9267.
"Wildcard imports"
This might be another personal preference thing, but I just don't like it. I
try eliminate them every time I see one :D
AoclDiagnosticOutputParser.java --> mostly valid comments (I didn't touch the
parsing logic on purpose, but these are small changes)
IntelFpgaOpenclPlugin.java
1. Another "*" to eliminate :)
2. "Do we need the conf Configuration object of AbstractFpgaVendorPlugin at
all?" - It's needed in the implementation of {{initPlugin()}}. But
{{setConf()}} / {{getConf()}} can go.
3. Exception object -> yep, let's log into the console
4. msg variable -> agree, unnecessary
I'll do these changes tomorrow.
> 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
> * checkstyle fixes
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
