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-9269) Minor cleanup in FpgaResourceAllocator
Date Thu, 07 Feb 2019 17:44:00 GMT

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


Mime
View raw message