peter-toth commented on code in PR #861: URL: https://github.com/apache/spark-kubernetes-operator/pull/861#discussion_r4073065942
########## docs/configuration.md: ########## @@ -82,8 +84,8 @@ events, since the resource stays in its initializing state without a state trans | Reason | When | |---|---| -| `SuspendHeld` | The resource is held by `spec.suspend`, so the driver (or master and worker) is not requested. It is republished every 30 minutes while the hold lasts (`spark.kubernetes.operator.reconciler.suspendHoldRequeueIntervalSeconds`), so a repeat bumps the `count` of the one event rather than creating another. Suspending a queued resource releases its Kueue `Workload`, and the message says so, since the `KueueAdmissionPending` event it was queued with outlives that `Workload`. | -| `KueueAdmissionPending` | The Kueue `Workload` waits for the admission. It is republished while it waits, so a repeat bumps the `count` of the one event rather than creating another. | +| `SuspendHeld` | The resource is held by `spec.suspend`, so the driver (or master and worker) is not requested. It is republished every 30 minutes while the hold lasts (`spark.kubernetes.operator.reconciler.suspendHoldRequeueIntervalSeconds`), so a repeat bumps the `count` of the one event rather than creating another. A repeat is also subject to [`minIntervalSeconds`](#event-frequency), which must be kept below the requeue interval. Suspending a queued resource releases its Kueue `Workload`, and the message says so, since the `KueueAdmissionPending` event it was queued with outlives that `Workload`; that repeat carries a new `message`, so it is published even within the interval. | Review Comment: **Finding 4.** "which must be kept below the requeue interval" is the rule this commit replaced everywhere else. `grep` finds it in exactly one place, so it is the fifth site rather than one of the four you counted: the `Event frequency` section, both option descriptions and the two generated `config_properties.md` rows all state the corrected bound. The argument against it is the one you made for the reciprocal clause in [issuecomment-5778311639](https://github.com/apache/spark-kubernetes-operator/pull/861#issuecomment-5778311639): `I <= R` only coincides with `I <= TTL - R` because `TTL - R == R` at the default of 1800. Lower the requeue interval and the two diverge fast, and it diverges in the direction that makes the row wrong rather than merely conservative. The repo's own configuration is the counterexample. `tests/e2e/helm/events-config-values.yaml` sets `suspendHoldRequeueIntervalSeconds=10` and `minIntervalSeconds=15`, which this row declares illegal, and it is correct — measured on the timed recorder: ``` I=15 R=10 period=20 first publishes=[0, 20, 40, 60] ``` 20 seconds against a 3600 second TTL, with every other repeat dropped, which is exactly the behaviour the E2E now relies on. Since the row already links to the section that states the rule properly, the shortest fix is to stop restating it: ```suggestion | `SuspendHeld` | The resource is held by `spec.suspend`, so the driver (or master and worker) is not requested. It is republished every 30 minutes while the hold lasts (`spark.kubernetes.operator.reconciler.suspendHoldRequeueIntervalSeconds`), so a repeat bumps the `count` of the one event rather than creating another. A repeat is also subject to [`minIntervalSeconds`](#event-frequency). Suspending a queued resource releases its Kueue `Workload`, and the message says so, since the `KueueAdmissionPending` event it was queued with outlives that `Workload`; that repeat carries a new `message`, so it is published even within the interval. | ``` -- 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]
