starcatmeow commented on PR #57484: URL: https://github.com/apache/spark/pull/57484#issuecomment-5074996263
@viirya Thanks for the thorough review and for checking these edge cases. Both non-blocking notes make sense. Since we may consolidate with #56957, I’ll keep them in mind for whichever implementation becomes the base. Thanks @pan3793 for pointing out #56957, and thanks everyone for the discussion. I reviewed #56957, and I agree that the two PRs overlap substantially and should be consolidated rather than maintained as two independent implementations. The main differences I see are: - #56957 introduces a general `TaskAssignmentStrategy` abstraction with `roundrobin`, `binpack`, `balance`, and `none`. Its bin-packing strategy sorts the initial `WorkerOffer.cores` snapshot in ascending order and keeps assigning to the current offer until it is exhausted. Since `WorkerOffer.cores` represents free cores, this approximates busy-first placement when executors have uniform capacity. - #57484 has a narrower scope: it keeps the existing spread behavior and adds only an opt-in bin-packing mode. It explicitly places executors that already have running tasks, or have received assignments earlier in the same `resourceOffers` call, before idle executors. It then uses a stable executor ID order within each group. - #56957 applies the selected strategy independently at every locality level, while #57484 limits bin-packing to `NO_PREF` and `ANY`. I think the broader locality behavior in #56957 is also reasonable because `TaskSetManager` still gates every launch by locality. For `PROCESS_LOCAL`, the eligible executor is normally already fixed; for `NODE_LOCAL`, the strategy may change which executor on the same host is filled first, but it does not reduce locality. I do not have a strong preference about which PR should be the base. If the maintainers prefer the more general assignment-strategy abstraction, I am happy to collaborate on #56957, port over any useful busy-state handling and test coverage from this PR, and close this PR. If a narrower change specifically targeting SPARK-43086 is preferred, we can use #57484 as the base and incorporate the relevant details from #56957. I am happy to follow the maintainers' guidance on the desired scope and locality behavior. @cloud-fan Happy to have a broader design discussion! I don’t want to claim that changing the scheduler is necessarily the best or only approach. My main question with autoscaler-driven draining is how to predict future executor demand. The current executor target seems like the most direct signal, but future demand is highly workload-dependent. Proactively decommissioning based on the target may require additional hysteresis, such as using its historical trend and only scaling down after it remains low for some time. The goal of bin-packing here is to reuse the existing executor idle timeout as a smoothing mechanism: after a wave of tasks finishes, executors are removed only if they remain unnecessary beyond the timeout, reducing scale-down/scale-up churn. This has worked well for our workloads, but I’m happy to discuss better end-to-end designs for dynamic allocation. -- 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]
