dongjoon-hyun commented on PR #854: URL: https://github.com/apache/spark-kubernetes-operator/pull/854#issuecomment-5758938083
Thank you for the thorough review, @peter-toth! All six are addressed in 2da0874. **1. Hold taken on a failed read.** `SparkAppContext.getCurrentAttemptDriverPodStrictly()` now surfaces a failed verification, and `getCurrentAttemptDriverPod()` keeps its lenient contract by wrapping it, so existing callers are unchanged. The suspend guard in `AppInitStep` reads the strict variant and, when the verification fails, returns `completeAndDefaultRequeue()` without publishing anything: an unknown answer is no longer read as "no driver requested", and no longer stretches the next look to the hold interval. `suspendedAppWithUnverifiableDriverIsNotHeld` covers it. The cluster half waits for the `getResourceStrictly` follow-up on #853, as you suggested. **2. Message claims a pending event that may never have existed.** `KueueWorkloadUtils.releaseWorkload` now reports whether a `Workload` was actually deleted, and the suffix is conditional on that. A resource created with `suspend: true` and a queue name therefore gets the plain message, which `suspendedAppWithQueueNameDoesNotCreateKueueWorkload` now asserts in full. **3. Hardcoded republish interval.** Added `spark.kubernetes.operator.reconciler.suspendHoldRequeueIntervalSeconds` (`Long`, default `1800`, dynamic override) next to `MISSING_DRIVER_REQUEUE_INTERVAL_SECONDS`, and its description points out that it must stay below the API server `--event-ttl`. `tests/e2e/helm/events-config-values.yaml` sets it to 10 seconds, so the `suspend-events` group asserts `count > 1` again for both resource kinds and the republishing is back under E2E coverage. **4. Nothing pins the documented 30 minutes.** Both test classes now build the expected progress from a `Duration.ofMinutes(30)` literal rather than from the value under test. Verified that it bites: setting the option default to 120 seconds fails the suspend-hold tests. **5.** Re-wrapped, the paragraph is back to 100 or under. **6.** Reworded to "a resource held by `spec.suspend` or by Kueue, whose status does not report the hold", which covers the `ScheduledToRestart` case as well. -- 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]
