dongjoon-hyun commented on PR #857: URL: https://github.com/apache/spark-kubernetes-operator/pull/857#issuecomment-5759301975
Thanks for the review, @peter-toth! All four findings are addressed in `3ab7498`, and the PR is rebased onto SPARK-59677 (#854), which landed first. **1 (Blocking)** — Fixed as suggested. Both admission branches now classify the failure like the `KueueWorkloadUtils.holdForAdmission` they sit in front of: `ReconcilerUtils.isTransientError` keeps `STALE_WORKLOAD_REQUEUE_INTERVAL` with no event, and everything else — 403 and 429 included — gets `completeAndDefaultRequeue()` plus a `KueueAdmissionRequestFailed` warning. The 403 arm is pinned by `refusedDriverLookupBeforeKueueAdmissionPublishesEvent` and `refusedMasterLookupBeforeKueueAdmissionPublishesEvent`, and the 503 tests now also assert `verifyNoInteractions(eventRecorder)`. The `docs/configuration.md` row for the reason is widened to cover the pre-admission read, as you noted. **2 (Non-blocking)** — Applied, with one correction: `private` does not compile. `ReconcilerUtilsTest` is in the same package, but package-private access is not `private`, so `compileTestJava` fails with `getResource(KubernetesClient,T) has private access in ReconcilerUtils`. It is package-private now, which still makes the misuse impossible from outside `org.apache.spark.k8s.operator.utils`, and its javadoc says it is the create loop's read. **3 (Minor)** — Took your wording. The PR description had the same gap and is updated as well. **4 (Minor)** — `warn` at all the new sites. On the sequencing: since #854 landed first, this PR is rebased onto it. The driver suspend branch is yours now, `isDriverRequested` moved to the `getCurrentAttemptDriverPodStrictly` it added, and my `SparkAppContext` change is gone. The cluster suspend branch mirrors your driver one: a failed read returns `completeAndDefaultRequeue()` without going through `SuspendUtils.holdForSuspend`, so no `SuspendHeld` event claims that no master was requested. One leftover follows from that: `SparkAppContext.getCurrentAttemptDriverPod` no longer has a production caller, since `isDriverRequested` was the last one — the same shape as your finding 2. I left the lenient variant alone and noted it in the description as a follow-up, since removing it touches the context and its tests rather than the two decisions this PR fixes. Happy to fold it in here if you would rather see it in one go. -- 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]
