ivoson commented on PR #37268: URL: https://github.com/apache/spark/pull/37268#issuecomment-1206460341
> ``` > /** > * Return resource profile Ids of executors where tasks can be assigned to. > */ > def compatibleExecutorRpIds(rpMgr: ResourceProfileManager): Set[Int] > ``` > > It seems a little bit odd to ask the ResourceProfile to give you compatible other ResourceProfiles. This feels like it should be in the ResourceProfileManager which knows about all the ResourceProfiles. I guess that is why you pass in the ResourceProfileManager here? Is the intention the user could explicitly set which ResourceProfiles its compatible with? If so I definitely would want a way to not have to specify it. Yes, exactly. I put the `ResourceProfileManager` here because it knows about all the ResourceProfiles. Adding this API just want to make sure that we have one interface to get compatible RP Ids for scheduler to assign tasks. And the implementation can be further enriched in future maybe for re-use executors with dynamic allocation on and adding more reuse policy as #33941 does. And we'll not let user to specify compatible ResourceProfiles. In this case, we will only have `TaskResourceProfile` compatible with `Default ResourceProfile`. > > The other issue raised that wasn't addressed was the reuse policy. I guess in this case we are limiting the executor profile to 1 because we don't have dynamic allocation so one could argue that if you use task resource request with that you know what you get. Which I am fine with but we need to be clear that it might very well waste resources. > > Also if the intent is to not support TaskResourceProfile with dynamic allocation, I think we should throw an exception if anyone uses it with the dynamic allocation config on. As mentioned above, in this case we will only have `TaskResourceProfile` re-use executors with `Default ResourceProfile` when dynamic allocation is off. The behavior will be limited to dynamic allocation off and will throw an exception if user use it with dynamic allocation on. Does this behavior change make sense? @tgravescs -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
