xintongsong edited a comment on issue #11320: [FLINK-16437][runtime] Make SlotManager allocate resource from ResourceManager at the worker granularity. URL: https://github.com/apache/flink/pull/11320#issuecomment-609556553 Thanks for the review, @tillrohrmann. Regarding the nullable `defaultWorkerResourceSpec` and `defaultSlotResourceProfile`, I'm trying to understand and summarize your concerns as follows. Please correct me if I'm wrong. - It is not explicit what the null values means and should be handled. - It makes `SlotManagerImpl` kind of inferring which deployment (standalone or active) it works on, and behave differently. These concerns make good sense to me. Actually, I have also considered having a special `WorkerResourceSpec.UNKNOWN`. My concern is that, it may require all the places that uses `WorkerResourceSpec` to check and handle the special value, which seems unnecessary to me. E.g., for `ResourceActions#allocateResource`, the argument `WorkerResourceProfile` should never be `UNKNOWN` because SM should always request workers from RM with specific worker resource spec. If we add `WorkerResourceSpec.UNKNOWN`, we have to either carefully make sure it is never passed to `allocateResource` in all the implementations of `SlotManager`, or check argument in all implementations of `ResourceAction`. I think `WorkerResourceSpec.UNKNOWN` is different from `ResourceProfile.UNKNOWN` or `ResourceSpec.UNKNOWN`. For `ResourceProfile`/`ResourceSpec`, `UNKNOWN` means we need some resource but we don't known how many exactly. For `defaultWorkerResourceSpec`, the semantic we actually need for standalone clusters is "the default worker does not exist", or in other words "by default we do not request workers". Combining your concerns and mine, and the above understanding, I would propose the following approach. - Introduce an `DefaultWorkerSlotResourceProvider`, with methods `Optional<WorkerResourceSpec> getDefaultWorkerResourceSpec` and `Optional<ResourceProfile> getDefaultSlotResourceProfile`. - For standalone clusters, we can pass a special `DefaultWorkerSlotResourceProvider.NO_DEFAULTS` to `SlotManagerImpl`, which returns `Optional.empty()` for both methods. The absent optional value clearly reflects the semantics of "not existing". - `SlotManagerImpl` no longer needs to assume which RM implementation it works with. It simply not allocate resource from RM if `DefaultWorkerSlotResourceProvider#getDefaultWorkerResourceSpec` returns empty. 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 With regards, Apache Git Services
