HwangDongJun commented on PR #58187: URL: https://github.com/apache/spark/pull/58187#issuecomment-5949269217
@dongjoon-hyun Thanks for the thorough review. Replies are up on each inline comment, and here's a full summary matching your numbering, so this comment stands on its own: **1. Regression for successful submissions** - [Fixed](https://github.com/apache/spark/pull/58187#discussion_r4163358689): the driver pod is created first, gated with `spec.schedulingGates`. Every pre-resource's single creation call already carries its owner reference (a CREATE, never an UPDATE), so `OwnerReferencesPermissionEnforcement` is never triggered. **2. Design** - [Addressed](https://github.com/apache/spark/pull/58187#discussion_r4163584199): implemented `spec.schedulingGates` as proposed -- no ownerless window, no shutdown hook, no second apply. - [Addressed](https://github.com/apache/spark/pull/58187#discussion_r4163597553): the hook (and its coverage gaps) is gone entirely rather than patched. **3. Failure handling (the shutdown hook these applied to no longer exists)** - [Moot](https://github.com/apache/spark/pull/58187#discussion_r4163373265): per-item cleanup never masks the exception being rethrown. - [Moot](https://github.com/apache/spark/pull/58187#discussion_r4163388586): no `finally` removing a hook anymore. - [Moot](https://github.com/apache/spark/pull/58187#discussion_r4163398733): no second thread, so no stale-flag race. - [Moot](https://github.com/apache/spark/pull/58187#discussion_r4163428383): no hook running blocking I/O during shutdown. - [Fixed](https://github.com/apache/spark/pull/58187#discussion_r4163435261): `deletePodAndPreResources` deletes every item independently. - [Fixed](https://github.com/apache/spark/pull/58187#discussion_r4163441250): `configMapNameDriver` is now a `def`, so same-JVM submissions can't share (and cross-delete) a name. **4. Impact on `apache/spark-kubernetes-operator`** - [Still applies, both counts](https://github.com/apache/spark/pull/58187#discussion_r4163449095) ([2](https://github.com/apache/spark/pull/58187#discussion_r4163471635)): this is the operator's own `AppInitStep`/`AppCleanUpStep` sequencing, independent of `Client.run()`. Noted as a known limitation in the PR description. **5. Tests and cleanups** - [Addressed](https://github.com/apache/spark/pull/58187#discussion_r4163569067): `InOrder` ordering, the `.edit()` lambda actually invoked and asserted on, a dedicated pod-creation-failure test, explicit matchers. - [Addressed](https://github.com/apache/spark/pull/58187#discussion_r4163575524): `KubernetesDriverBuilderSuite`'s `SPARK-37331` tests now exclude `HadoopConfDriverFeatureStep`. - [Moot](https://github.com/apache/spark/pull/58187#discussion_r4163609340): the duplicated client creation and `ShutdownHookOps` are gone with the hook. - [Addressed](https://github.com/apache/spark/pull/58187#discussion_r4163622553): the comments these applied to are gone; also caught and fixed a couple of the same pattern that crept back into `ClientSuite`. Also updated the PR description: corrected the `VolcanoFeatureStep` claim, replaced the hook description with the `spec.schedulingGates` design, updated the test count, and added a "Known limitations" section (operator impact above, and the `nodeName`→`nodeSelector` fallback's taint-bypass trade-off). Let me know if anything above needs another look. -- 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]
