dongjoon-hyun commented on PR #58216: URL: https://github.com/apache/spark/pull/58216#issuecomment-5383650676
Thanks for the detailed writeup and for including a repro test. The failure mode you describe -- the `executorManagement` queue drops a `SparkListenerStageSubmitted`, dynamic allocation silently stops requesting executors, and the UI keeps showing the stage as `RUNNING` -- is real and worth documenting. That said, I don't think we need a new metric for it, because the per-queue dropped-event counter is already exported. `AsyncEventQueue` registers `queue.<name>.numDroppedEvents` on the `LiveListenerBusMetrics` registry when the queue is created: https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/scheduler/AsyncEventQueue.scala#L85 and that source (`sourceName = "LiveListenerBus"`) is registered with the `MetricsSystem` in `LiveListenerBus.start()`: https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/scheduler/LiveListenerBus.scala#L182 So `<app-id>.driver.LiveListenerBus.queue.executorManagement.numDroppedEvents.count` is already available today in JMX/Prometheus/any configured sink. The proposed `ExecutorAllocationManagerSource` gauge returns exactly that same counter under a second name. It is also strictly less available than the existing one: `ExecutorAllocationManagerSource` is only registered when `spark.dynamicAllocation.enabled=true` https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/SparkContext.scala#L747-L749 whereas the `LiveListenerBus` source is always registered. What I believe is the actual gap here is documentation. In `docs/monitoring.md`, the `namespace=LiveListenerBus` list spells out all three metrics for `appStatus` and `eventLog`, but stops at `queue.executorManagement.listenerProcessingTime`. `queue.executorManagement.numDroppedEvents.count`, `queue.executorManagement.size`, and the entire `queue.shared.*` group are missing: https://github.com/apache/spark/blob/master/docs/monitoring.md?plain=1#L1327-L1338 If an operator could not find this signal, that omission seems the likely reason. Would you consider re-scoping this PR to fill in those missing lines? That makes exactly the signal you needed discoverable, with no new API surface and no duplicated metric. A few other notes, in case parts of the change are kept: - The description says the only existing signal is "a generic, easy-to-miss `WARN` log line". `AsyncEventQueue` also logs an `ERROR` on the first drop, including the queue name: https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/scheduler/AsyncEventQueue.scala#L182-L187 - `metrics.metricRegistry.counter(...)` has get-or-add semantics, so the new accessor registers a permanent all-zero counter as a side effect if the `executorManagement` queue does not exist yet. It also duplicates the `queue.<name>.numDroppedEvents` name string in a second place, so a rename in `AsyncEventQueue` would make it silently return 0. Reading through `metricRegistry.getCounters.get(...)`, or delegating to the queue itself, avoids both. - The new `ExecutorAllocationManagerSuite` test looks racy. `new ResourceProfileManager(conf, customBus)` posts `SparkListenerResourceProfileAdded` from its constructor (`ResourceProfileManager.scala` L59-L60 and L159). With `capacity=1`, if the dispatch thread has not taken that event before `post(SparkListenerJobStart(0, ...))` runs, `JobStart(0)` is the event that gets dropped and `blockStarted.acquire()` blocks forever. A `customBus.waitUntilEmpty()` right after creating the `ResourceProfileManager` would make it deterministic. - `assert(manager.maxNumExecutorsNeededPerResourceProfile(defaultProfile.id) === 0)` passes both when the drop caused the stall and when nothing happened at all. A control case with a large capacity (expecting 2) would make the causality explicit. Note also that this assertion pins the current buggy behavior, so it has to be removed once the underlying issue is actually fixed; worth calling out in a comment. - If a gauge does stay, please follow the naming used by the rest of `ExecutorAllocationManagerSource` (`numberMaxNeededExecutors`, `numberDecommissioningExecutors`, ...), and add it to the `namespace=ExecutorAllocationManager` list in `docs/monitoring.md` since the description marks this as a user-facing change. -- 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]
