wangyum commented on PR #58968: URL: https://github.com/apache/spark/pull/58968#issuecomment-5865544134
> Thank you for updating the PR, @wangyum. I reviewed the latest commit ([90dda44](https://github.com/apache/spark/commit/90dda44fe000335eb8ef7f6355f21c369b53b4d9)). > > The production change is a behavior-preserving refactoring, and the latest commit addresses my previous comments. Summary of the remaining comments: > > 1. **PR description**: The `How was this patch tested?` section is outdated. The structural test now lives in `LiveEntitySuite` instead of `AppStatusListenerSuite`. It checks a field-type allow-list and whether `addTaskMetrics` increments every declared `Long` field. Since the PR description becomes the commit message, could you update it? For example: > ``` > `AppStatusListenerSuite`: > - A live stage receives `SparkListenerExecutorMetricsUpdate` and then `SparkListenerTaskEnd`, each with distinct metric values. The test checks all 10 `ExecutorStageSummary` metric fields after each event. > > `LiveEntitySuite`: > - `LiveExecutorStageSummary` declares only `Long`, `Int`, `Boolean`, `String`, or `ExecutorMetrics` fields. > - `addTaskMetrics` increments every declared `Long` field except `taskTime`. > ``` > 2. **Test (nit)**: `buildMetrics` leaves the push-based merged bytes at zero. An extra term such as `remoteMergedBytesRead` in `addTaskMetrics` would not be caught. > 3. **Pre-existing (not introduced by this PR)**: `speculationStageSummary.numActiveTasks` can become negative on `Resubmitted`. This is out of scope and can be handled with the `taskTime` issue in a separate JIRA. Fix these issues. -- 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]
