[
https://issues.apache.org/jira/browse/YARN-9269?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16762899#comment-16762899
]
Adam Antal commented on YARN-9269:
----------------------------------
Thanks for the patch, [~pbacsko]. Nice job, the quality of the code is much
better now. Here are some thoughts of mine:
- Could you please rationale why has the behaviour of addFpga changed?
Previously the devices from the variable list (also there can be some more
descriptive name of it) is processed this way: devices are being added one by
one to allowedFpgas list, if they're not already added. Now we collect them
into a list named fpgaDevices, and at the end of the function we overwrite the
list allowedFpgas, so the devices added to the allowedFpgas list before the
function is called can be lost.
- I agree that the function findMatchedFpga is not needed (basically just a
type of search/"indexOf" function), but the updateFpga function's side effect
is that it also checks whether usedFpgaByRequestor (renamed to
containerToFpgaMapping) contains that device or not, and if not contained a
warning is raised. Even though this function is only called from
FpgaResourceHandlerImpl:preStart, where we know that this case does not happen,
I think we should explicitly keep that.
- There are some unused function in FpgaDevice class: setDevName,
setAliasDevName, setBusNum. It makes sense to have getters and setters for all
the class variables, but they're also missing from temperature, and card power
usage, so I think the above mentioned function could be removed.
- It's really a minor thing, but can we rename the class variable availableFpga
to availableFpgas (so an extra s, implying its more than one device).
- There are also useless extra spaces on the top of the file, right after the
license and after the package statement.
Please enlight me, if I don't see something clear.
> Minor cleanup in FpgaResourceAllocator
> --------------------------------------
>
> Key: YARN-9269
> URL: https://issues.apache.org/jira/browse/YARN-9269
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Peter Bacsko
> Assignee: Peter Bacsko
> Priority: Major
> Attachments: YARN-9269-001.patch, YARN-9269-002.patch
>
>
> Some stuff that we observed:
> * {{addFpga()}} - we check for duplicate devices, but we don't print any
> error/warning if there's any.
> * {{findMatchedFpga()}} should be called {{findMatchingFpga()}}. Also, is
> this method even needed? We already receive an {{FpgaDevice}} instance in
> {{updateFpga()}} which I believe is the same that we're looking up.
> * variable {{IPIDpreference}} is confusing
> * {{availableFpga}} / {{usedFpgaByRequestor}} are instances of
> {{LinkedHashMap}}. What's the rationale behind this? Doesn't a simple
> {{HashMap}} suffice?
> * {{usedFpgaByRequestor}} should be renamed, naming is a bit unclear
> * {{allowedFpgas}} should be an immutable list
> * {{@VisibleForTesting}} methods should be package private
> * get rid of {{*}} imports
--
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
