[ https://issues.apache.org/jira/browse/YARN-9318?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16778600#comment-16778600 ]
Daniel Templeton commented on YARN-9318: ---------------------------------------- Thanks for the patch, [~snemeth]. I have a few comments: # Your {{applyFunctionOnValues(Resource lhs, Resource rhs, BiFunction<Long, Long, Long> valueFunction)}} method adds complication for no gain. You're still duplicating the logic, so you're better off leaving {{multiplyAndAddTo()}} as it was. # I'm kinda on the fence on the use of the lambdas. You only ever pass in two lambdas: {{(value) -> (long) (value * by)}} and {{(value) -> (long) Math.ceil(value * by)}}. The alternative would be to pass in a boolean that says whether to round up or down. It would be easier to read but less flexible. I don't see the multiplication functions changing that much in the future, so I would suggest the boolean route. # You've changed the behavior of the {{multiply*()}} methods. In the original code, if a resource type causes an error, the operation aborts. In the new code it keeps going. I personally think both behaviors are wrong; I think the exception should be ducked. Regardless, let's not change behavior arbitrarily. # In {{TestResources}} you should fix the name of {{testMultipleRoundUp()}} to be {{testMultiplyRoundUp()}}. # In {{testMultiplyAndRoundUpCustomResources()}} it would be great to have some assert messages. While you're at it, it would also be nice to have more useful assert messages in {{testMultipleRoundUp()}}. > Resources#multiplyAndRoundUp does not consider Resource Types > ------------------------------------------------------------- > > Key: YARN-9318 > URL: https://issues.apache.org/jira/browse/YARN-9318 > Project: Hadoop YARN > Issue Type: Bug > Reporter: Szilard Nemeth > Assignee: Szilard Nemeth > Priority: Major > Attachments: YARN-9318.001.patch, YARN-9318.002.patch > > > org.apache.hadoop.yarn.util.resource.Resources#multiplyAndRoundUp only deals > with memory and vcores while computing the rounded value. It should also > consider custom Resource Types as well. -- 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