sunchao commented on PR #58131: URL: https://github.com/apache/spark/pull/58131#issuecomment-5347697248
Thanks, @dongjoon-hyun, and thanks @viirya for tracing both race orderings. I pushed the follow-up in [4f67717](https://github.com/apache/spark/commit/4f67717fa8f8bff7aa843f6ade563323403fb851). On the API default, I agree that declining the request is a tradeoff, especially for a custom client that implements `ExecutorAllocationClient` directly. The contract now explicitly says that such a client must override the idle-only operation to support graceful dynamic-allocation scale-down. All in-tree clients inherit the coarse-grained implementation. I would prefer to keep the existing three-argument method intact in this patch. The idle-only wrapper filters under the scheduler/backend locks and then calls that method through virtual dispatch, so Kubernetes and existing custom coarse-backend overrides still receive the accepted IDs. Replacing the signature with a `force` parameter would require updating those overrides; a compatibility-preserving overload would still need the same filtering bridge and default behavior. A non-forced `killExecutors` fallback is possible, but it would bypass a custom client's existing graceful implementation and use abrupt removal instead. I chose to decline unsupported requests rather than make that behavior change implicitly. If you prefer the overload shape for API consistency, I can adapt it while preserving those guarantees. On the remaining points, idle-only requests intentionally do not enter the unknown-executor replay cache, while explicit requests retain their SPARK-41766 behavior. I kept deduplication because the old path can accept the first occurrence, then treat the second as inactive and queue it for replay; `distinctBy` makes that protection clearer. The follow-up also makes the Kubernetes retention setting test-local and constructs the backend after applying it, replaces the broad Mockito assertion with an exact check of every executor-ID selector, documents the mailbox-flush barrier, reuses `MOCK_REGEX`, and removes the extra blank line. All 83 focused tests and the four style checks passed, including a final rerun after restoring two negative controls. Removing the busy check failed both ordinary/barrier race tests; adding a mixed accepted/rejected executor-ID selector failed the focused Kubernetes assertion. -- 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]
