KarmaGYZ commented on a change in pull request #14897:
URL: https://github.com/apache/flink/pull/14897#discussion_r573574434
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/util/ResourceCounter.java
##########
@@ -35,11 +37,12 @@
* associated counts. The counts are always positive (> 0).
*/
public final class ResourceCounter {
Review comment:
Thanks a lot for the explanation. I now understand your point.
> passing these objects around and then mutating it will cause other
components to see these changes.
Agreed. When the `ResourceCounter` used as an internal state, we need to
ensure it will not be exposed outside. If a method has to return a
`ResourceCounter` instance, it should realize this can be modified outside.
> Also you don't have to worry about concurrency and these kind of things.
Regarding the concern of concurrency modification, I'm not quite sure about
this. If we make the `ResourceCounter` immutable, then we need to change its
reference every time we add/substract `ResourceProfile` to it. This means we
might need a lock to protect it and make it `volatile`. Just making it
immutable may not solve the problem.
An additional idea inspired by your comment. Maybe the `ResourceCounter`
should only be used as a `Counter` instead of a `Record`. When a method needs
to return a set of resources, something like immutable `ResourceRequirement`
might be more suitable. When someone want to count a set of resources, they
need mutable `ResourceCounter` instead. WDYT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
