xintongsong commented 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`. However, I think this 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". My concern is that, `WorkerResourceSpec` is also used as the argument type of `ResourceActions#allocateResource`. With the current design, SM should always request workers from RM with specific worker resource spec. If we add WorkerResourceSpec.UNKNOWN, we have to carefully make sure it is never passed to `allocateResource`. Combining your concerns and mine, 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: [email protected] With regards, Apache Git Services
